From df46b9004b0ac81d6cd37a644e5c8243af0afc61 Mon Sep 17 00:00:00 2001 From: Pawel Wodnicki Date: Thu, 22 Nov 2012 06:21:31 +0000 Subject: Merging r168291: into the 3.2 release branch. Fix PR14060, an infinite loop in reassociate. The problem was that one of the operands of the expression being written was wrongly thought to be reusable as an inner node of the expression resulting in it turning up as both an inner node *and* a leaf, creating a cycle in the def-use graph. This would have caused the verifier to blow up if things had gotten that far, however it managed to provoke an infinite loop first. git-svn-id: https://llvm.org/svn/llvm-project/llvm/branches/release_32@168489 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Transforms/Scalar/Reassociate.cpp | 30 ++++++++++++++++++++++++------ test/Transforms/Reassociate/crash.ll | 19 +++++++++++++++++++ 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/lib/Transforms/Scalar/Reassociate.cpp b/lib/Transforms/Scalar/Reassociate.cpp index f19b7fec0a..7a4079784b 100644 --- a/lib/Transforms/Scalar/Reassociate.cpp +++ b/lib/Transforms/Scalar/Reassociate.cpp @@ -606,8 +606,8 @@ void Reassociate::RewriteExprTree(BinaryOperator *I, SmallVectorImpl &Ops) { assert(Ops.size() > 1 && "Single values should be used directly!"); - // Since our optimizations never increase the number of operations, the new - // expression can always be written by reusing the existing binary operators + // Since our optimizations should never increase the number of operations, the + // new expression can usually be written reusing the existing binary operators // from the original expression tree, without creating any new instructions, // though the rewritten expression may have a completely different topology. // We take care to not change anything if the new expression will be the same @@ -621,6 +621,20 @@ void Reassociate::RewriteExprTree(BinaryOperator *I, unsigned Opcode = I->getOpcode(); BinaryOperator *Op = I; + /// NotRewritable - The operands being written will be the leaves of the new + /// expression and must not be used as inner nodes (via NodesToRewrite) by + /// mistake. Inner nodes are always reassociable, and usually leaves are not + /// (if they were they would have been incorporated into the expression and so + /// would not be leaves), so most of the time there is no danger of this. But + /// in rare cases a leaf may become reassociable if an optimization kills uses + /// of it, or it may momentarily become reassociable during rewriting (below) + /// due it being removed as an operand of one of its uses. Ensure that misuse + /// of leaf nodes as inner nodes cannot occur by remembering all of the future + /// leaves and refusing to reuse any of them as inner nodes. + SmallPtrSet NotRewritable; + for (unsigned i = 0, e = Ops.size(); i != e; ++i) + NotRewritable.insert(Ops[i].Op); + // 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. @@ -653,12 +667,14 @@ void Reassociate::RewriteExprTree(BinaryOperator *I, // the old operands with the new ones. DEBUG(dbgs() << "RA: " << *Op << '\n'); if (NewLHS != OldLHS) { - if (BinaryOperator *BO = isReassociableOp(OldLHS, Opcode)) + BinaryOperator *BO = isReassociableOp(OldLHS, Opcode); + if (BO && !NotRewritable.count(BO)) NodesToRewrite.push_back(BO); Op->setOperand(0, NewLHS); } if (NewRHS != OldRHS) { - if (BinaryOperator *BO = isReassociableOp(OldRHS, Opcode)) + BinaryOperator *BO = isReassociableOp(OldRHS, Opcode); + if (BO && !NotRewritable.count(BO)) NodesToRewrite.push_back(BO); Op->setOperand(1, NewRHS); } @@ -682,7 +698,8 @@ void Reassociate::RewriteExprTree(BinaryOperator *I, Op->swapOperands(); } else { // Overwrite with the new right-hand side. - if (BinaryOperator *BO = isReassociableOp(Op->getOperand(1), Opcode)) + BinaryOperator *BO = isReassociableOp(Op->getOperand(1), Opcode); + if (BO && !NotRewritable.count(BO)) NodesToRewrite.push_back(BO); Op->setOperand(1, NewRHS); ExpressionChanged = Op; @@ -695,7 +712,8 @@ void Reassociate::RewriteExprTree(BinaryOperator *I, // Now deal with the left-hand side. If this is already an operation node // from the original expression then just rewrite the rest of the expression // into it. - if (BinaryOperator *BO = isReassociableOp(Op->getOperand(0), Opcode)) { + BinaryOperator *BO = isReassociableOp(Op->getOperand(0), Opcode); + if (BO && !NotRewritable.count(BO)) { Op = BO; continue; } diff --git a/test/Transforms/Reassociate/crash.ll b/test/Transforms/Reassociate/crash.ll index a617e9f327..e29b5dc9c0 100644 --- a/test/Transforms/Reassociate/crash.ll +++ b/test/Transforms/Reassociate/crash.ll @@ -153,3 +153,22 @@ define i32 @bar(i32 %arg, i32 %arg1, i32 %arg2) { %ret = add i32 %tmp2, %tmp3 ret i32 %ret } + +; PR14060 +define i8 @hang(i8 %p, i8 %p0, i8 %p1, i8 %p2, i8 %p3, i8 %p4, i8 %p5, i8 %p6, i8 %p7, i8 %p8, i8 %p9) { + %tmp = zext i1 false to i8 + %tmp16 = or i8 %tmp, 1 + %tmp22 = or i8 %p7, %p0 + %tmp23 = or i8 %tmp16, %tmp22 + %tmp28 = or i8 %p9, %p1 + %tmp31 = or i8 %tmp23, %p2 + %tmp32 = or i8 %tmp31, %tmp28 + %tmp38 = or i8 %p8, %p3 + %tmp39 = or i8 %tmp16, %tmp38 + %tmp43 = or i8 %tmp39, %p4 + %tmp44 = or i8 %tmp43, 1 + %tmp47 = or i8 %tmp32, %p5 + %tmp50 = or i8 %tmp47, %p6 + %tmp51 = or i8 %tmp44, %tmp50 + ret i8 %tmp51 +} -- cgit v1.2.3