Since commit 157467, if reassociate isn't actually going to change an expression

then it doesn't alter the instructions composing it, however it would continue
to move the instructions to just before the expression root.  Ensure it doesn't
move them either, so now it really does nothing if there is nothing to do.  That
commit also ensured that nsw etc flags weren't cleared if the expression was not
being changed.  Tweak this a bit so that it doesn't clear flags on the initial
part of a computation either if that part didn't change but later bits did.

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

diff --git a/lib/Transforms/Scalar/Reassociate.cpp b/lib/Transforms/Scalar/Reassociate.cpp
index 7a9a41c950a49b775e282c3b56ea7d68a0bd55fa..0da70b37615328197b5547e998e80858c77913ff 100644 (file)
--- a/lib/Transforms/Scalar/Reassociate.cpp
+++ b/lib/Transforms/Scalar/Reassociate.cpp
@@ -308,7 +308,8 @@ static BinaryOperator *LowerNegateToMultiply(Instruction *Neg,
 /// NOTE: This routine will set operands of non-leaf non-root nodes to undef in
 /// order to ensure that every non-root node in the expression has *exactly one*
 /// use by a non-leaf node of the expression.  This destruction means that the
-/// caller MUST use something like RewriteExprTree to put the values back in.
+/// caller MUST either replace 'I' with a new expression or use something like
+/// RewriteExprTree to put the values back in.
 ///
 /// In the above example either the right operand of A or the left operand of B
 /// will be replaced by undef.  If it is B's operand then this gives:
@@ -321,8 +322,9 @@ static BinaryOperator *LowerNegateToMultiply(Instruction *Neg,
 ///                / \ / \ / \   |
 ///                   +   *      |      F,  G
 ///
-/// Note that if you visit operands recursively starting from a leaf node then
-/// you will never encounter such an undef operand unless you get back to 'I',
+/// Note that such undef operands can only be reached by passing through 'I'.
+/// For example, if you visit operands recursively starting from a leaf node
+/// then you will never see such an undef operand unless you get back to 'I',
 /// which requires passing through a phi node.
 ///
 /// Note that this routine may also mutate binary operators of the wrong type
@@ -508,18 +510,19 @@ void Reassociate::RewriteExprTree(BinaryOperator *I,
   unsigned Opcode = I->getOpcode();
   NodesToRewrite.push_back(I);
 
-  // ExpressionChanged - Whether the rewritten expression differs non-trivially
-  // from the original, requiring the clearing of all optional flags.
-  bool ExpressionChanged = false;
+  // ExpressionChanged - Non-null if the rewritten expression differs from the
+  // 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) {
     assert(!NodesToRewrite.empty() &&
            "Optimized expressions has more nodes than original!");
     Previous = Op; Op = NodesToRewrite.pop_back_val();
-    // Compactify the tree instructions together with each other to guarantee
-    // that the expression tree is dominated by all of Ops.
-    if (Previous)
+    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);
 
     // The last operation (which comes earliest in the IR) is special as both
@@ -560,7 +563,7 @@ void Reassociate::RewriteExprTree(BinaryOperator *I,
       }
       DEBUG(dbgs() << "TO: " << *Op << '\n');
 
-      ExpressionChanged = true;
+      ExpressionChanged = Op;
       MadeChange = true;
       ++NumChanged;
 
@@ -581,7 +584,7 @@ void Reassociate::RewriteExprTree(BinaryOperator *I,
         if (BinaryOperator *BO = isReassociableOp(Op->getOperand(1), Opcode))
           NodesToRewrite.push_back(BO);
         Op->setOperand(1, NewRHS);
-        ExpressionChanged = true;
+        ExpressionChanged = Op;
       }
       DEBUG(dbgs() << "TO: " << *Op << '\n');
       MadeChange = true;
@@ -603,19 +606,19 @@ void Reassociate::RewriteExprTree(BinaryOperator *I,
     DEBUG(dbgs() << "RA: " << *Op << '\n');
     Op->setOperand(0, NodesToRewrite.back());
     DEBUG(dbgs() << "TO: " << *Op << '\n');
-    ExpressionChanged = true;
+    ExpressionChanged = Op;
     MadeChange = true;
     ++NumChanged;
   }
 
-  // If the expression changed non-trivially then clear out all subclass data in
-  // the entire rewritten expression.
+  // If the expression changed non-trivially then clear out all subclass data
+  // starting from the operator specified in ExpressionChanged.
   if (ExpressionChanged) {
     do {
-      Op->clearSubclassOptionalData();
-      if (Op == I)
+      ExpressionChanged->clearSubclassOptionalData();
+      if (ExpressionChanged == I)
         break;
-      Op = cast<BinaryOperator>(*Op->use_begin());
+      ExpressionChanged = cast<BinaryOperator>(*ExpressionChanged->use_begin());
     } while (1);
   }
 

diff --git a/test/Transforms/Reassociate/no-op.ll b/test/Transforms/Reassociate/no-op.ll
new file mode 100644 (file)
index 0000000..0444cf0
--- /dev/null
+++ b/test/Transforms/Reassociate/no-op.ll
@@ -0,0 +1,38 @@
+; RUN: opt < %s -reassociate -S | FileCheck %s
+
+; When there is nothing to do, or not much to do, check that reassociate leaves
+; things alone.
+
+declare void @use(i32)
+
+define void @test1(i32 %a, i32 %b) {
+; Shouldn't change or move any of the add instructions.  Should commute but
+; otherwise not change or move any of the mul instructions.
+; CHECK: @test1
+  %a0 = add nsw i32 %a, 1
+; CHECK-NEXT: %a0 = add nsw i32 %a, 1
+  %m0 = mul nsw i32 3, %a
+; CHECK-NEXT: %m0 = mul nsw i32 %a, 3
+  %a1 = add nsw i32 %a0, %b
+; CHECK-NEXT: %a1 = add nsw i32 %a0, %b
+  %m1 = mul nsw i32 %b, %m0
+; CHECK-NEXT: %m1 = mul nsw i32 %m0, %b
+  call void @use(i32 %a1)
+; CHECK-NEXT: call void @use
+  call void @use(i32 %m1)
+  ret void
+}
+
+define void @test2(i32 %a, i32 %b, i32 %c, i32 %d) {
+; The initial add doesn't change so should not lose the nsw flag.
+; CHECK: @test2
+  %a0 = add nsw i32 %b, %a
+; CHECK-NEXT: %a0 = add nsw i32 %b, %a
+  %a1 = add nsw i32 %a0, %d
+; CHECK-NEXT: %a1 = add i32 %a0, %c
+  %a2 = add nsw i32 %a1, %c
+; CHECK-NEXT: %a2 = add i32 %a1, %d
+  call void @use(i32 %a2)
+; CHECK-NEXT: call void @use
+  ret void
+}