Merge pull request #213 from kernigh/kernigh-bug-203

Fix #203: ack -O3 crash from strength reduction

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();
+}

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"
 				}
 			}
 

util/ego/build.lua

@@ -9,7 +9,7 @@ local function build_ego(name)
 			"h+emheaders",
 		},
 		vars = {
-			["+cflags"] = {"-DVERBOSE", "-DNOTCOMPACT"}
+			["+cflags"] = {"-DDEBUG", "-DVERBOSE", "-DNOTCOMPACT"}
 		}
 	}
 end

util/ego/share/build.lua

@@ -55,6 +55,6 @@ clibrary {
 		"modules/src/em_data+lib",
 	},
 	vars = {
-		["+cflags"] = {"-DVERBOSE", "-DNOTCOMPACT"}
+		["+cflags"] = {"-DDEBUG", "-DVERBOSE", "-DNOTCOMPACT"}
 	}
 }

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();
 	}
 }

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);
 	}
 }

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);
 	}
 }
 

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;