diff --git a/lib/Transforms/Scalar/Reassociate.cpp b/lib/Transforms/Scalar/Reassociate.cpp index 7a9a41c950a..0da70b37615 100644 --- 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(*Op->use_begin()); + ExpressionChanged = cast(*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 index 00000000000..0444cf082d0 --- /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 +}