Some reassociate optimizations create new instructions, which they insert just

before the expression root.  Any existing operators that are changed to use one
of them needs to be moved between it and the expression root, and recursively
for the operators using that one.  When I rewrote RewriteExprTree I accidentally
inverted the logic, resulting in the compacting going down from operators to
operands rather than up from operands to the operators using them, oops.  Fix
this, resolving PR12963.


git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@159265 91177308-0d34-0410-b5e6-96231b3b80d8

2 files changed, 21 insertions, 11 deletions

diff --git a/lib/Transforms/Scalar/Reassociate.cpp b/lib/Transforms/Scalar/Reassociate.cpp
index b572f4e..69cdc0f 100644
--- a/lib/Transforms/Scalar/Reassociate.cpp
+++ b/lib/Transforms/Scalar/Reassociate.cpp
@@ -673,16 +673,10 @@ void Reassociate::RewriteExprTree(BinaryOperator *I,
   // original in some non-trivial way, requiring the clearing of optional flags.
   // Flags are cleared from the operator in ExpressionChanged up to I inclusive.
   BinaryOperator *ExpressionChanged = 0;
-  BinaryOperator *Previous;
-  BinaryOperator *Op = 0;
-  for (unsigned i = 0, e = Ops.size(); i != e; ++i) {
+  for (unsigned i = 0; ; ++i) {
     assert(!NodesToRewrite.empty() &&
            "Optimized expressions has more nodes than original!");
-    Previous = Op; Op = NodesToRewrite.pop_back_val();
-    if (ExpressionChanged)
-      // Compactify the tree instructions together with each other to guarantee
-      // that the expression tree is dominated by all of Ops.
-      Op->moveBefore(Previous);
+    BinaryOperator *Op = NodesToRewrite.pop_back_val();
 
     // The last operation (which comes earliest in the IR) is special as both
     // operands will come from Ops, rather than just one with the other being
@@ -771,15 +765,17 @@ void Reassociate::RewriteExprTree(BinaryOperator *I,
   }
 
   // If the expression changed non-trivially then clear out all subclass data
-  // starting from the operator specified in ExpressionChanged.
-  if (ExpressionChanged) {
+  // starting from the operator specified in ExpressionChanged, and compactify
+  // the operators to just before the expression root to guarantee that the
+  // expression tree is dominated by all of Ops.
+  if (ExpressionChanged)
     do {
       ExpressionChanged->clearSubclassOptionalData();
       if (ExpressionChanged == I)
         break;
+      ExpressionChanged->moveBefore(I);
       ExpressionChanged = cast<BinaryOperator>(*ExpressionChanged->use_begin());
     } while (1);
-  }
 
   // Throw away any left over nodes from the original expression.
   for (unsigned i = 0, e = NodesToRewrite.size(); i != e; ++i)
diff --git a/test/Transforms/Reassociate/crash.ll b/test/Transforms/Reassociate/crash.ll
index e2ebcdd..a99c844 100644
--- a/test/Transforms/Reassociate/crash.ll
+++ b/test/Transforms/Reassociate/crash.ll
@@ -119,3 +119,17 @@ for.cond:                                         ; preds = %for.cond, %entry
   %conv = zext i16 %p to i32
   br label %for.cond
 }
+
+; PR12963
+@a = external global i8
+define i8 @f0(i8 %x) {
+  %t0 = load i8* @a
+  %t1 = mul i8 %x, %x
+  %t2 = mul i8 %t1, %t1
+  %t3 = mul i8 %t2, %t2
+  %t4 = mul i8 %t3, %x
+  %t5 = mul i8 %t4, %t4
+  %t6 = mul i8 %t5, %x
+  %t7 = mul i8 %t6, %t0
+  ret i8 %t7
+}