[llvm-commits] [llvm] r159265 - in /llvm/trunk: lib/Transforms/Scalar/Reassociate.cpp test/Transforms/Reassociate/crash.ll

[Duncan Sands]

Author: baldrick
Date: Wed Jun 27 09:19:00 2012
New Revision: 159265

URL: http://llvm.org/viewvc/llvm-project?rev=159265&view=rev
Log:
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.

Modified:
    llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp
    llvm/trunk/test/Transforms/Reassociate/crash.ll

Modified: llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp?rev=159265&r1=159264&r2=159265&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp Wed Jun 27 09:19:00 2012
@@ -673,16 +673,10 @@
   // 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 @@
   }
 
   // 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)

Modified: llvm/trunk/test/Transforms/Reassociate/crash.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Reassociate/crash.ll?rev=159265&r1=159264&r2=159265&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/Reassociate/crash.ll (original)
+++ llvm/trunk/test/Transforms/Reassociate/crash.ll Wed Jun 27 09:19:00 2012
@@ -119,3 +119,17 @@
   %conv = zext i16 %p to i32
   br label %for.cond
 }
+
+; PR12963
+ at 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
+}