<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi Cameron,<div class=""><br class=""></div><div class="">This patch breaks some of internal testing. I filed <a href="https://bugs.llvm.org/show_bug.cgi?id=42349" class="">https://bugs.llvm.org/show_bug.cgi?id=42349</a> for a reproducer.</div><div class=""><br class=""></div><div class="">Basically we hit the assertion that FNeg doesn’t have the fast flag. So either the assertion is bogus or we miss a place that needs to check the flag is here.</div><div class=""><br class=""></div><div class="">I let you check.</div><div class=""><br class=""></div><div class="">Cheers,</div><div class="">-Quentin<br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Jun 19, 2019, at 7:59 AM, Cameron McInally via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">Author: mcinally<br class="">Date: Wed Jun 19 07:59:14 2019<br class="">New Revision: 363813<br class=""><br class="">URL: <a href="http://llvm.org/viewvc/llvm-project?rev=363813&view=rev" class="">http://llvm.org/viewvc/llvm-project?rev=363813&view=rev</a><br class="">Log:<br class="">[Reassociate] Handle unary FNeg in the Reassociate pass<br class=""><br class="">Differential Revision: <a href="https://reviews.llvm.org/D63445" class="">https://reviews.llvm.org/D63445</a><br class=""><br class="">Modified:<br class=""> llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp<br class=""> llvm/trunk/test/Transforms/Reassociate/fast-basictest.ll<br class=""><br class="">Modified: llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp<br class="">URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp?rev=363813&r1=363812&r2=363813&view=diff" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp?rev=363813&r1=363812&r2=363813&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp (original)<br class="">+++ llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp Wed Jun 19 07:59:14 2019<br class="">@@ -266,12 +266,20 @@ static BinaryOperator *CreateNeg(Value *<br class=""><br class=""> /// Replace 0-X with X*-1.<br class=""> static BinaryOperator *LowerNegateToMultiply(Instruction *Neg) {<br class="">+ assert((isa<UnaryOperator>(Neg) || isa<BinaryOperator>(Neg)) &&<br class="">+ "Expected a Negate!");<br class="">+ // It's not safe to lower a unary FNeg into a FMul by -1.0. However,<br class="">+ // we can only reach this function with fast flags set, so it's<br class="">+ // safe to do with nnan.<br class="">+ assert((!isa<FPMathOperator>(Neg) || Neg->isFast()) &&<br class="">+ "Expecting FastMathFlags!");<br class="">+ unsigned OpNo = isa<BinaryOperator>(Neg) ? 1 : 0;<br class=""> Type *Ty = Neg->getType();<br class=""> Constant *NegOne = Ty->isIntOrIntVectorTy() ?<br class=""> ConstantInt::getAllOnesValue(Ty) : ConstantFP::get(Ty, -1.0);<br class=""><br class="">- BinaryOperator *Res = CreateMul(Neg->getOperand(1), NegOne, "", Neg, Neg);<br class="">- Neg->setOperand(1, Constant::getNullValue(Ty)); // Drop use of op.<br class="">+ BinaryOperator *Res = CreateMul(Neg->getOperand(OpNo), NegOne, "", Neg, Neg);<br class="">+ Neg->setOperand(OpNo, Constant::getNullValue(Ty)); // Drop use of op.<br class=""> Res->takeName(Neg);<br class=""> Neg->replaceAllUsesWith(Res);<br class=""> Res->setDebugLoc(Neg->getDebugLoc());<br class="">@@ -444,8 +452,10 @@ using RepeatedValue = std::pair<Value*,<br class=""> /// that have all uses inside the expression (i.e. only used by non-leaf nodes<br class=""> /// of the expression) if it can turn them into binary operators of the right<br class=""> /// type and thus make the expression bigger.<br class="">-static bool LinearizeExprTree(BinaryOperator *I,<br class="">+static bool LinearizeExprTree(Instruction *I,<br class=""> SmallVectorImpl<RepeatedValue> &Ops) {<br class="">+ assert((isa<UnaryOperator>(I) || isa<BinaryOperator>(I)) &&<br class="">+ "Expected a UnaryOperator or BinaryOperator!");<br class=""> LLVM_DEBUG(dbgs() << "LINEARIZE: " << *I << '\n');<br class=""> unsigned Bitwidth = I->getType()->getScalarType()->getPrimitiveSizeInBits();<br class=""> unsigned Opcode = I->getOpcode();<br class="">@@ -462,7 +472,7 @@ static bool LinearizeExprTree(BinaryOper<br class=""> // with their weights, representing a certain number of paths to the operator.<br class=""> // If an operator occurs in the worklist multiple times then we found multiple<br class=""> // ways to get to it.<br class="">- SmallVector<std::pair<BinaryOperator*, APInt>, 8> Worklist; // (Op, Weight)<br class="">+ SmallVector<std::pair<Instruction*, APInt>, 8> Worklist; // (Op, Weight)<br class=""> Worklist.push_back(std::make_pair(I, APInt(Bitwidth, 1)));<br class=""> bool Changed = false;<br class=""><br class="">@@ -489,10 +499,10 @@ static bool LinearizeExprTree(BinaryOper<br class=""> SmallPtrSet<Value *, 8> Visited; // For sanity checking the iteration scheme.<br class=""> #endif<br class=""> while (!Worklist.empty()) {<br class="">- std::pair<BinaryOperator*, APInt> P = Worklist.pop_back_val();<br class="">+ std::pair<Instruction*, APInt> P = Worklist.pop_back_val();<br class=""> I = P.first; // We examine the operands of this binary operator.<br class=""><br class="">- for (unsigned OpIdx = 0; OpIdx < 2; ++OpIdx) { // Visit operands.<br class="">+ for (unsigned OpIdx = 0; OpIdx < I->getNumOperands(); ++OpIdx) { // Visit operands.<br class=""> Value *Op = I->getOperand(OpIdx);<br class=""> APInt Weight = P.second; // Number of paths to this operand.<br class=""> LLVM_DEBUG(dbgs() << "OPERAND: " << *Op << " (" << Weight << ")\n");<br class="">@@ -572,14 +582,14 @@ static bool LinearizeExprTree(BinaryOper<br class=""><br class=""> // If this is a multiply expression, turn any internal negations into<br class=""> // multiplies by -1 so they can be reassociated.<br class="">- if (BinaryOperator *BO = dyn_cast<BinaryOperator>(Op))<br class="">- if ((Opcode == Instruction::Mul && match(BO, m_Neg(m_Value()))) ||<br class="">- (Opcode == Instruction::FMul && match(BO, m_FNeg(m_Value())))) {<br class="">+ if (Instruction *Tmp = dyn_cast<Instruction>(Op))<br class="">+ if ((Opcode == Instruction::Mul && match(Tmp, m_Neg(m_Value()))) ||<br class="">+ (Opcode == Instruction::FMul && match(Tmp, m_FNeg(m_Value())))) {<br class=""> LLVM_DEBUG(dbgs()<br class=""> << "MORPH LEAF: " << *Op << " (" << Weight << ") TO ");<br class="">- BO = LowerNegateToMultiply(BO);<br class="">- LLVM_DEBUG(dbgs() << *BO << '\n');<br class="">- Worklist.push_back(std::make_pair(BO, Weight));<br class="">+ Tmp = LowerNegateToMultiply(Tmp);<br class="">+ LLVM_DEBUG(dbgs() << *Tmp << '\n');<br class="">+ Worklist.push_back(std::make_pair(Tmp, Weight));<br class=""> Changed = true;<br class=""> continue;<br class=""> }<br class="">@@ -2020,7 +2030,7 @@ Instruction *ReassociatePass::canonicali<br class=""> /// instructions is not allowed.<br class=""> void ReassociatePass::OptimizeInst(Instruction *I) {<br class=""> // Only consider operations that we understand.<br class="">- if (!isa<BinaryOperator>(I))<br class="">+ if (!isa<UnaryOperator>(I) && !isa<BinaryOperator>(I))<br class=""> return;<br class=""><br class=""> if (I->getOpcode() == Instruction::Shl && isa<ConstantInt>(I->getOperand(1)))<br class="">@@ -2085,7 +2095,8 @@ void ReassociatePass::OptimizeInst(Instr<br class=""> I = NI;<br class=""> }<br class=""> }<br class="">- } else if (I->getOpcode() == Instruction::FSub) {<br class="">+ } else if (I->getOpcode() == Instruction::FNeg ||<br class="">+ I->getOpcode() == Instruction::FSub) {<br class=""> if (ShouldBreakUpSubtract(I)) {<br class=""> Instruction *NI = BreakUpSubtract(I, RedoInsts);<br class=""> RedoInsts.insert(I);<br class="">@@ -2094,7 +2105,9 @@ void ReassociatePass::OptimizeInst(Instr<br class=""> } else if (match(I, m_FNeg(m_Value()))) {<br class=""> // Otherwise, this is a negation. See if the operand is a multiply tree<br class=""> // and if this is not an inner node of a multiply tree.<br class="">- if (isReassociableOp(I->getOperand(1), Instruction::FMul) &&<br class="">+ Value *Op = isa<BinaryOperator>(I) ? I->getOperand(1) :<br class="">+ I->getOperand(0);<br class="">+ if (isReassociableOp(Op, Instruction::FMul) &&<br class=""> (!I->hasOneUse() ||<br class=""> !isReassociableOp(I->user_back(), Instruction::FMul))) {<br class=""> // If the negate was simplified, revisit the users to see if we can<br class=""><br class="">Modified: llvm/trunk/test/Transforms/Reassociate/fast-basictest.ll<br class="">URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Reassociate/fast-basictest.ll?rev=363813&r1=363812&r2=363813&view=diff" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Reassociate/fast-basictest.ll?rev=363813&r1=363812&r2=363813&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/test/Transforms/Reassociate/fast-basictest.ll (original)<br class="">+++ llvm/trunk/test/Transforms/Reassociate/fast-basictest.ll Wed Jun 19 07:59:14 2019<br class="">@@ -586,12 +586,10 @@ define float @test18(float %a, float %b,<br class=""> ret float %f<br class=""> }<br class=""><br class="">-; FIXME: This reassociation is not working.<br class=""> define float @test18_unary_fneg(float %a, float %b, float %z) {<br class=""> ; CHECK-LABEL: @test18_unary_fneg(<br class="">-; CHECK-NEXT: [[C:%.*]] = fmul fast float [[Z:%.*]], -4.000000e+01<br class="">-; CHECK-NEXT: [[E:%.*]] = fmul fast float [[C]], [[A:%.*]]<br class="">-; CHECK-NEXT: [[F:%.*]] = fneg fast float [[E]]<br class="">+; CHECK-NEXT: [[E:%.*]] = fmul fast float [[A:%.*]], 4.000000e+01<br class="">+; CHECK-NEXT: [[F:%.*]] = fmul fast float [[E]], [[Z:%.*]]<br class=""> ; CHECK-NEXT: ret float [[F]]<br class=""> ;<br class=""> %d = fmul fast float %z, 4.000000e+01<br class="">@@ -616,6 +614,21 @@ define float @test18_reassoc(float %a, f<br class=""> ret float %f<br class=""> }<br class=""><br class="">+; It is not safe to reassociate unary fneg without nnan.<br class="">+define float @test18_reassoc_unary_fneg(float %a, float %b, float %z) {<br class="">+; CHECK-LABEL: @test18_reassoc_unary_fneg(<br class="">+; CHECK-NEXT: [[C:%.*]] = fmul reassoc float [[Z:%.*]], -4.000000e+01<br class="">+; CHECK-NEXT: [[E:%.*]] = fmul reassoc float [[C]], [[A:%.*]]<br class="">+; CHECK-NEXT: [[F:%.*]] = fneg reassoc float [[E]]<br class="">+; CHECK-NEXT: ret float [[F]]<br class="">+;<br class="">+ %d = fmul reassoc float %z, 4.000000e+01<br class="">+ %c = fneg reassoc float %d<br class="">+ %e = fmul reassoc float %a, %c<br class="">+ %f = fneg reassoc float %e<br class="">+ ret float %f<br class="">+}<br class="">+<br class=""> ; With sub reassociation, constant folding can eliminate the 12 and -12 constants.<br class=""> define float @test19(float %A, float %B) {<br class=""> ; CHECK-LABEL: @test19(<br class=""><br class=""><br class="">_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a><br class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits<br class=""></div></div></blockquote></div><br class=""></div></body></html>