From fc1476c88bb1b0d8ccacad8438351ce95fb42b10 Mon Sep 17 00:00:00 2001 From: George Koehler Date: Thu, 14 Nov 2019 16:17:17 -0500 Subject: [PATCH 1/3] Add -DDEBUG to enable assertions in util/ego Our build enables assertions in some other ACK tools (like assemblers and LLgen), but disabled them in ego until now. Bug #203 becomes a failed assertion in ego's SR phase: sr: util/ego/sr/sr_reduce.c:483: fix_header: Assertion `INSTR(e) == ps_end' failed. Comment out 2 assertions in util/ego/share, because they fail on systems with 4-byte int, 8-byte long. --- util/ego/build.lua | 2 +- util/ego/share/build.lua | 2 +- util/ego/share/get.c | 3 ++- util/ego/share/put.c | 3 ++- 4 files changed, 6 insertions(+), 4 deletions(-) diff --git a/util/ego/build.lua b/util/ego/build.lua index 78895f508..97f177822 100644 --- a/util/ego/build.lua +++ b/util/ego/build.lua @@ -9,7 +9,7 @@ local function build_ego(name) "h+emheaders", }, vars = { - ["+cflags"] = {"-DVERBOSE", "-DNOTCOMPACT"} + ["+cflags"] = {"-DDEBUG", "-DVERBOSE", "-DNOTCOMPACT"} } } end diff --git a/util/ego/share/build.lua b/util/ego/share/build.lua index 586cce928..7e152364a 100644 --- a/util/ego/share/build.lua +++ b/util/ego/share/build.lua @@ -55,6 +55,6 @@ clibrary { "modules/src/em_data+lib", }, vars = { - ["+cflags"] = {"-DVERBOSE", "-DNOTCOMPACT"} + ["+cflags"] = {"-DDEBUG", "-DVERBOSE", "-DNOTCOMPACT"} } } diff --git a/util/ego/share/get.c b/util/ego/share/get.c index 6cc695471..84a805828 100644 --- a/util/ego/share/get.c +++ b/util/ego/share/get.c @@ -83,7 +83,8 @@ STATIC int getint(void) if (sizeof(int) == sizeof(short)) { return getshort(); } else { - assert (sizeof(int) == sizeof(offset)); + /* Fails with 4-byte int, 8-byte long: + * assert (sizeof(int) == sizeof(offset)); */ return getoff(); } } diff --git a/util/ego/share/put.c b/util/ego/share/put.c index 2ce0c3863..13907147e 100644 --- a/util/ego/share/put.c +++ b/util/ego/share/put.c @@ -113,7 +113,8 @@ STATIC void outint(int i) if (sizeof(int) == sizeof(short)) { outshort(i); } else { - assert (sizeof(int) == sizeof(offset)); + /* Fails with 4-byte int, 8-byte long: + * assert (sizeof(int) == sizeof(offset)); */ outoff((offset) i); } } From e841adf97020e57cda9f4c88432b251a0ba6c69b Mon Sep 17 00:00:00 2001 From: George Koehler Date: Fri, 15 Nov 2019 11:50:05 -0500 Subject: [PATCH 2/3] Fix END pseudo in util/ego/sr; closes #203 The strength reduction (SR) phase mishandled the END pseudo when SR found at least 2 different expressions in the same loop, and SR made a new header for the loop. Since 1987 (commit 159b84e), SR appended the new header to the end of the procedure. Later, SR fixed the header by adding a BRA branch to the loop's entry, and moving the END pseudo from the previous basic block to the header. If SR found multiple expressions, it called fix_header() multiple times, and tried to move the END again. The extra move failed assert(INSTR(e) == ps_end), or moved another line after the END, so opt2 (the peephole optimizer) would crash. Fix by removing fix_header() and moving the code to make_header(), so SR adds the BRA and moves the END when it makes the header, and does so only once for each header. Adjust init_code(), which inserts code into a header. Stop checking for an empty header, because make_header() now adds BRA. After inserting code before BRA, don't move LP_INSTR, because later insertions should go before BRA, not after END. --- util/ego/sr/sr_reduce.c | 62 ++++++++--------------------------------- util/ego/sr/sr_xform.c | 32 +++++++++++++++++---- 2 files changed, 37 insertions(+), 57 deletions(-) diff --git a/util/ego/sr/sr_reduce.c b/util/ego/sr/sr_reduce.c index c78e456a1..7480f900c 100644 --- a/util/ego/sr/sr_reduce.c +++ b/util/ego/sr/sr_reduce.c @@ -171,9 +171,7 @@ STATIC line_p add_code(pl, l) -STATIC void init_code(code,tmp) - code_p code; - offset tmp; +STATIC void init_code(code_p code, offset tmp) { /* Generate code to set up the temporary local. * For multiplication, its initial value is const*iv_expr, @@ -223,20 +221,19 @@ STATIC void init_code(code,tmp) assert(FALSE); /* non-reducible instruction */ } PREV(l->l_next) = l; + /* Now insert the code at the end of the header block */ p = &code->co_loop->LP_INSTR; - if (*p == (line_p) 0 || (PREV((*p)) == 0 && INSTR((*p)) == op_bra)) { - /* LP_INSTR points to last instruction of header block, - * so if it is 0, the header block is empty yet. - */ - code->co_loop->LP_HEADER->b_start = - add_code(code->co_loop->LP_HEADER->b_start, code->co_lfirst); - } else if (INSTR((*p)) == op_bra) { + if (INSTR((*p)) == op_bra) { + /* Add code before branching to the loop. */ add_code(PREV((*p)), code->co_lfirst); + } else { + /* Add code before falling into the loop. */ + add_code(*p, code->co_lfirst); + while (l->l_next) + l = l->l_next; + *p = l; /* new last instruction */ } - else add_code(*p, code->co_lfirst); - while (l->l_next) l = l->l_next; - *p = l; /* new last instruction */ } STATIC void incr_code(code,tmp) @@ -453,43 +450,7 @@ STATIC code_p available(c,vars) return (code_p) 0; } -STATIC void fix_header(lp) - loop_p lp; -{ - /* Check if a header block was added, and if so, add a branch to - * the entry block. - * If it was added, it was added to the end of the procedure, so - * move the END pseudo. - */ - bblock_p b = curproc->p_start; - - if (lp->LP_HEADER->b_next == 0) { - line_p l = last_instr(lp->LP_HEADER); - line_p e; - - assert(l != 0); - if (INSTR(l) != op_bra) { - line_p j = newline(OPINSTRLAB); - - assert(INSTR(lp->lp_entry->b_start) == op_lab); - INSTRLAB(j) = INSTRLAB(lp->lp_entry->b_start); - j->l_instr = op_bra; - DLINK(l, j); - l = j; - } - - while (b->b_next != lp->LP_HEADER) b = b->b_next; - e = last_instr(b); - assert(INSTR(e) == ps_end); - assert(PREV(e) != 0); - PREV(e)->l_next = 0; - DLINK(l, e); - } -} - -STATIC void reduce(code,vars) - code_p code; - lset vars; +STATIC void reduce(code_p code, lset vars) { /* Perform the actual transformations. The code on the left * gets transformed into the code on the right. Note that @@ -538,7 +499,6 @@ STATIC void reduce(code,vars) incr_code(code,tmp); /* emit code to increment temp. local */ OUTTRACE("emitted increment code",0); Ladd(code,&avail); - fix_header(code->co_loop); } } diff --git a/util/ego/sr/sr_xform.c b/util/ego/sr/sr_xform.c index d48a70844..5323ceacb 100644 --- a/util/ego/sr/sr_xform.c +++ b/util/ego/sr/sr_xform.c @@ -138,9 +138,7 @@ STATIC void adjust_jump(newtarg,oldtarg,c) } -void -make_header(lp) - loop_p lp; +void make_header(loop_p lp) { /* Make sure that the loop has a header block, i.e. a block * has the loop entry block as its only successor and @@ -149,6 +147,7 @@ make_header(lp) */ bblock_p b,c,entry; + line_p branch,last; Lindex i,next; if (lp->LP_HEADER != (bblock_p) 0) return; @@ -160,6 +159,17 @@ make_header(lp) b = freshblock(); /* new block with new b_id */ entry = lp->lp_entry; + /* In the header, add a branch from to the entry block. */ + branch = newline(OPINSTRLAB); + assert(INSTR(entry->b_start) == op_lab); + INSTRLAB(branch) = INSTRLAB(entry->b_start); + branch->l_instr = op_bra; + b->b_start = branch; + + /* Plan to insert code before the branch. */ + assert(lp->LP_INSTR == 0); + lp->LP_INSTR = branch; + /* update succ/pred. Also take care that any jump from outside * the loop to the entry block now goes to b. */ @@ -180,14 +190,24 @@ make_header(lp) adjust_jump(b,entry,c); } } - assert(lp->LP_INSTR == 0); - lp->LP_INSTR = b->b_start; + Ladd(b,&entry->b_pred); Ladd(entry,&b->b_succ); + /* put header block at end of procedure */ - for (c = curproc->p_start; c->b_next != 0; c = c->b_next); + for (c = curproc->p_start; c->b_next != 0; c = c->b_next) + continue; c->b_next = b; /* b->b_next = 0; */ + + /* move the END pseudo to header block */ + last = last_instr(c); + assert(INSTR(last) == ps_end); + assert(PREV(last) != (line_p) 0); + PREV(last)->l_next = 0; + DLINK(branch, last); + + /* fix loops and dominance */ copy_loops(b,entry,lp); b->b_idom = entry->b_idom; entry->b_idom = b; From 32e60ea9944041dc182c0484f0a3df89b58dbf65 Mon Sep 17 00:00:00 2001 From: George Koehler Date: Fri, 15 Nov 2019 15:33:15 -0500 Subject: [PATCH 3/3] Add test case for #203 left_shift() adapts the example from @davidgiven that caused `ack -O3` to crash. multiply() is a similar example. Edit the build system to use -O3 for this test. It now takes -O3 from the test's filename, and still defaults to -O0. --- tests/plat/bugs/bug-203-ego-sr_c-O3.c | 47 +++++++++++++++++++++++++++ tests/plat/build.lua | 12 +++++-- 2 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 tests/plat/bugs/bug-203-ego-sr_c-O3.c diff --git a/tests/plat/bugs/bug-203-ego-sr_c-O3.c b/tests/plat/bugs/bug-203-ego-sr_c-O3.c new file mode 100644 index 000000000..9e851effb --- /dev/null +++ b/tests/plat/bugs/bug-203-ego-sr_c-O3.c @@ -0,0 +1,47 @@ +#include "test.h" + +/* + * https://github.com/davidgiven/ack/issues/203 was a bug in the + * strength reduction (SR) phase of ego. To cause the bug: + * + * 1. SR must run; it runs at `ack -O3` and higher. + * + * 2. SR must find a loop with an induction variable, and SR must + * find at least 2 different expressions using the variable, like + * i << 1, (i + 1) << 1, or 3 * i, 5 * i. + * + * 3. There is no basic block to act as a loop header, so SR must + * create the block. One way is to put if (one) { ... } around + * the loop, so the loop is just after a conditional branch. + */ + +/* + * Global variables prevent optimizations: + * if (one) { ... } caused the bug, if (1) { ... } didn't. + */ +int zero = 0; +int one = 1; +short ary[] = {8, 8, 8, 8, 8, 9}; + +int left_shift(int i) { + if (one) { + /* EM will use i << 1, (i + 1) << 1 here. */ + while (ary[i] == ary[i + 1]) + i++; + } + return i; +} + +int multiply(int i) { + if (one) { + while (((3 * i) & (5 * i)) < 40) + i++; + } + return i; +} + +int main(void) { + ASSERT(left_shift(zero) == 4); + ASSERT(multiply(zero) == 21); + finished(); +} diff --git a/tests/plat/build.lua b/tests/plat/build.lua index e5318eae4..0ae4a5c19 100644 --- a/tests/plat/build.lua +++ b/tests/plat/build.lua @@ -5,7 +5,7 @@ definerule("plat_testsuite", { plat = { type="string" }, method = { type="string" }, - -- added long-long/llswitch_e.c + -- added bugs/bug-203-ego-sr_c-O3.c sets = { type="table", default={"core", "b", "bugs", "m2", "floats", "long-long"}}, skipsets = { type="table", default={}}, tests = { type="targets", default={} }, @@ -48,6 +48,14 @@ definerule("plat_testsuite", lang = "e" end + -- If lang is "c-O3", then build with -O3. + local _, _, optimize = lang:find("(-O[^-]*)$") + if optimize then + lang = lang:sub(1, -1 - #optimize) + else + optimize = "-O0" + end + local bin = ackprogram { name = fs.."_bin", srcs = { f }, @@ -55,7 +63,7 @@ definerule("plat_testsuite", vars = { plat = e.plat, lang = lang, - ackcflags = "-O0 -Bmain" + ackcflags = optimize.." -Bmain" } }