From b8ab9c6736422261a0e8dd37c84e81a162c9403c Mon Sep 17 00:00:00 2001 From: QingShan Zhang Date: Wed, 20 May 2020 02:11:16 +0000 Subject: [PATCH] [DAGCombine] Remove the getNegatibleCost to avoid the out of sync with getNegatedExpression We have the getNegatibleCost/getNegatedExpression to evaluate the cost and negate the expression. However, during negating the expression, the cost might change as we are changing the DAG, and then, hit the assertion if we negated the wrong expression as the cost is not trustful anymore. This patch is target to remove the getNegatibleCost to avoid the out of sync with getNegatedExpression, and check the cost during negating the expression. It also reduce the duplicated code between getNegatibleCost and getNegatedExpression. And fix the crash for the test in D76638 Reviewed By: RKSimon, spatel Differential Revision: https://reviews.llvm.org/D77319 --- include/llvm/CodeGen/TargetLowering.h | 34 +- lib/CodeGen/SelectionDAG/TargetLowering.cpp | 329 +++++++++----------- lib/Target/AMDGPU/AMDGPUISelLowering.cpp | 14 +- lib/Target/AMDGPU/AMDGPUISelLowering.h | 7 +- lib/Target/X86/X86ISelLowering.cpp | 74 +---- lib/Target/X86/X86ISelLowering.h | 18 +- test/CodeGen/X86/neg_fp.ll | 27 ++ 7 files changed, 222 insertions(+), 281 deletions(-) diff --git a/include/llvm/CodeGen/TargetLowering.h b/include/llvm/CodeGen/TargetLowering.h index 58da3fa1c71..2cab2572335 100644 --- a/include/llvm/CodeGen/TargetLowering.h +++ b/include/llvm/CodeGen/TargetLowering.h @@ -3545,37 +3545,27 @@ public: llvm_unreachable("Not Implemented"); } - /// Returns whether computing the negated form of the specified expression is - /// more expensive, the same cost or cheaper. - virtual NegatibleCost getNegatibleCost(SDValue Op, SelectionDAG &DAG, - bool LegalOperations, bool ForCodeSize, - unsigned Depth = 0) const; - - /// If getNegatibleCost returns Neutral/Cheaper, return the newly negated - /// expression. - virtual SDValue negateExpression(SDValue Op, SelectionDAG &DAG, bool LegalOps, - bool OptForSize, unsigned Depth = 0) const; - /// Return the newly negated expression if the cost is not expensive and /// set the cost in \p Cost to indicate that if it is cheaper or neutral to /// do the negation. - SDValue getNegatedExpression(SDValue Op, SelectionDAG &DAG, bool LegalOps, - bool OptForSize, NegatibleCost &Cost, - unsigned Depth = 0) const { - Cost = getNegatibleCost(Op, DAG, LegalOps, OptForSize, Depth); - if (Cost != NegatibleCost::Expensive) - return negateExpression(Op, DAG, LegalOps, OptForSize, Depth); - return SDValue(); - } + virtual SDValue getNegatedExpression(SDValue Op, SelectionDAG &DAG, + bool LegalOps, bool OptForSize, + NegatibleCost &Cost, + unsigned Depth = 0) const; /// This is the helper function to return the newly negated expression only /// when the cost is cheaper. SDValue getCheaperNegatedExpression(SDValue Op, SelectionDAG &DAG, bool LegalOps, bool OptForSize, unsigned Depth = 0) const { - if (getNegatibleCost(Op, DAG, LegalOps, OptForSize, Depth) == - NegatibleCost::Cheaper) - return negateExpression(Op, DAG, LegalOps, OptForSize, Depth); + NegatibleCost Cost = NegatibleCost::Expensive; + SDValue Neg = + getNegatedExpression(Op, DAG, LegalOps, OptForSize, Cost, Depth); + if (Neg && Cost == NegatibleCost::Cheaper) + return Neg; + // Remove the new created node to avoid the side effect to the DAG. + if (Neg && Neg.getNode()->use_empty()) + DAG.RemoveDeadNode(Neg.getNode()); return SDValue(); } diff --git a/lib/CodeGen/SelectionDAG/TargetLowering.cpp b/lib/CodeGen/SelectionDAG/TargetLowering.cpp index 00a7a87a553..a8890788ccb 100644 --- a/lib/CodeGen/SelectionDAG/TargetLowering.cpp +++ b/lib/CodeGen/SelectionDAG/TargetLowering.cpp @@ -5562,165 +5562,79 @@ verifyReturnAddressArgumentIsConstant(SDValue Op, SelectionDAG &DAG) const { return false; } -TargetLowering::NegatibleCost -TargetLowering::getNegatibleCost(SDValue Op, SelectionDAG &DAG, - bool LegalOperations, bool ForCodeSize, - unsigned Depth) const { +SDValue TargetLowering::getNegatedExpression(SDValue Op, SelectionDAG &DAG, + bool LegalOps, bool OptForSize, + NegatibleCost &Cost, + unsigned Depth) const { // fneg is removable even if it has multiple uses. - if (Op.getOpcode() == ISD::FNEG) - return NegatibleCost::Cheaper; - - // Don't allow anything with multiple uses unless we know it is free. - EVT VT = Op.getValueType(); - const SDNodeFlags Flags = Op->getFlags(); - const TargetOptions &Options = DAG.getTarget().Options; - if (!Op.hasOneUse()) { - bool IsFreeExtend = Op.getOpcode() == ISD::FP_EXTEND && - isFPExtFree(VT, Op.getOperand(0).getValueType()); - - // If we already have the use of the negated floating constant, it is free - // to negate it even it has multiple uses. - bool IsFreeConstant = - Op.getOpcode() == ISD::ConstantFP && - !negateExpression(Op, DAG, LegalOperations, ForCodeSize).use_empty(); - - if (!IsFreeExtend && !IsFreeConstant) - return NegatibleCost::Expensive; + if (Op.getOpcode() == ISD::FNEG) { + Cost = NegatibleCost::Cheaper; + return Op.getOperand(0); } // Don't recurse exponentially. if (Depth > SelectionDAG::MaxRecursionDepth) - return NegatibleCost::Expensive; + return SDValue(); - switch (Op.getOpcode()) { + // Pre-increment recursion depth for use in recursive calls. + ++Depth; + const SDNodeFlags Flags = Op->getFlags(); + const TargetOptions &Options = DAG.getTarget().Options; + EVT VT = Op.getValueType(); + unsigned Opcode = Op.getOpcode(); + + // Don't allow anything with multiple uses unless we know it is free. + if (!Op.hasOneUse() && Opcode != ISD::ConstantFP) { + bool IsFreeExtend = Opcode == ISD::FP_EXTEND && + isFPExtFree(VT, Op.getOperand(0).getValueType()); + if (!IsFreeExtend) + return SDValue(); + } + + SDLoc DL(Op); + + switch (Opcode) { case ISD::ConstantFP: { - if (!LegalOperations) - return NegatibleCost::Neutral; - // Don't invert constant FP values after legalization unless the target says // the negated constant is legal. - if (isOperationLegal(ISD::ConstantFP, VT) || + bool IsOpLegal = + isOperationLegal(ISD::ConstantFP, VT) || isFPImmLegal(neg(cast(Op)->getValueAPF()), VT, - ForCodeSize)) - return NegatibleCost::Neutral; - break; + OptForSize); + + if (LegalOps && !IsOpLegal) + break; + + APFloat V = cast(Op)->getValueAPF(); + V.changeSign(); + SDValue CFP = DAG.getConstantFP(V, DL, VT); + + // If we already have the use of the negated floating constant, it is free + // to negate it even it has multiple uses. + if (!Op.hasOneUse() && CFP.use_empty()) + break; + Cost = NegatibleCost::Neutral; + return CFP; } case ISD::BUILD_VECTOR: { // Only permit BUILD_VECTOR of constants. if (llvm::any_of(Op->op_values(), [&](SDValue N) { return !N.isUndef() && !isa(N); })) - return NegatibleCost::Expensive; - if (!LegalOperations) - return NegatibleCost::Neutral; - if (isOperationLegal(ISD::ConstantFP, VT) && - isOperationLegal(ISD::BUILD_VECTOR, VT)) - return NegatibleCost::Neutral; - if (llvm::all_of(Op->op_values(), [&](SDValue N) { + break; + + bool IsOpLegal = + (isOperationLegal(ISD::ConstantFP, VT) && + isOperationLegal(ISD::BUILD_VECTOR, VT)) || + llvm::all_of(Op->op_values(), [&](SDValue N) { return N.isUndef() || isFPImmLegal(neg(cast(N)->getValueAPF()), VT, - ForCodeSize); - })) - return NegatibleCost::Neutral; - break; - } - case ISD::FADD: { - if (!Options.NoSignedZerosFPMath && !Flags.hasNoSignedZeros()) - return NegatibleCost::Expensive; + OptForSize); + }); - // After operation legalization, it might not be legal to create new FSUBs. - if (LegalOperations && !isOperationLegalOrCustom(ISD::FSUB, VT)) - return NegatibleCost::Expensive; + if (LegalOps && !IsOpLegal) + break; - // fold (fneg (fadd A, B)) -> (fsub (fneg A), B) - NegatibleCost V0 = getNegatibleCost(Op.getOperand(0), DAG, LegalOperations, - ForCodeSize, Depth + 1); - if (V0 != NegatibleCost::Expensive) - return V0; - // fold (fneg (fadd A, B)) -> (fsub (fneg B), A) - return getNegatibleCost(Op.getOperand(1), DAG, LegalOperations, ForCodeSize, - Depth + 1); - } - case ISD::FSUB: - // We can't turn -(A-B) into B-A when we honor signed zeros. - if (!Options.NoSignedZerosFPMath && !Flags.hasNoSignedZeros()) - return NegatibleCost::Expensive; - - // fold (fneg (fsub A, B)) -> (fsub B, A) - return NegatibleCost::Neutral; - case ISD::FMUL: - case ISD::FDIV: { - // fold (fneg (fmul X, Y)) -> (fmul (fneg X), Y) or (fmul X, (fneg Y)) - NegatibleCost V0 = getNegatibleCost(Op.getOperand(0), DAG, LegalOperations, - ForCodeSize, Depth + 1); - if (V0 != NegatibleCost::Expensive) - return V0; - - // Ignore X * 2.0 because that is expected to be canonicalized to X + X. - if (auto *C = isConstOrConstSplatFP(Op.getOperand(1))) - if (C->isExactlyValue(2.0) && Op.getOpcode() == ISD::FMUL) - return NegatibleCost::Expensive; - - return getNegatibleCost(Op.getOperand(1), DAG, LegalOperations, ForCodeSize, - Depth + 1); - } - case ISD::FMA: - case ISD::FMAD: { - if (!Options.NoSignedZerosFPMath && !Flags.hasNoSignedZeros()) - return NegatibleCost::Expensive; - - // fold (fneg (fma X, Y, Z)) -> (fma (fneg X), Y, (fneg Z)) - // fold (fneg (fma X, Y, Z)) -> (fma X, (fneg Y), (fneg Z)) - NegatibleCost V2 = getNegatibleCost(Op.getOperand(2), DAG, LegalOperations, - ForCodeSize, Depth + 1); - if (NegatibleCost::Expensive == V2) - return NegatibleCost::Expensive; - - // One of Op0/Op1 must be cheaply negatible, then select the cheapest. - NegatibleCost V0 = getNegatibleCost(Op.getOperand(0), DAG, LegalOperations, - ForCodeSize, Depth + 1); - NegatibleCost V1 = getNegatibleCost(Op.getOperand(1), DAG, LegalOperations, - ForCodeSize, Depth + 1); - NegatibleCost V01 = std::min(V0, V1); - if (V01 == NegatibleCost::Expensive) - return NegatibleCost::Expensive; - return std::min(V01, V2); - } - - case ISD::FP_EXTEND: - case ISD::FP_ROUND: - case ISD::FSIN: - return getNegatibleCost(Op.getOperand(0), DAG, LegalOperations, ForCodeSize, - Depth + 1); - } - - return NegatibleCost::Expensive; -} - -SDValue TargetLowering::negateExpression(SDValue Op, SelectionDAG &DAG, - bool LegalOps, bool OptForSize, - unsigned Depth) const { - // fneg is removable even if it has multiple uses. - if (Op.getOpcode() == ISD::FNEG) - return Op.getOperand(0); - - assert(Depth <= SelectionDAG::MaxRecursionDepth && - "negateExpression doesn't match getNegatibleCost"); - - // Pre-increment recursion depth for use in recursive calls. - ++Depth; - const SDNodeFlags Flags = Op->getFlags(); - EVT VT = Op.getValueType(); - unsigned Opcode = Op.getOpcode(); - SDLoc DL(Op); - - switch (Opcode) { - case ISD::ConstantFP: { - APFloat V = cast(Op)->getValueAPF(); - V.changeSign(); - return DAG.getConstantFP(V, DL, VT); - } - case ISD::BUILD_VECTOR: { SmallVector Ops; for (SDValue C : Op->op_values()) { if (C.isUndef()) { @@ -5731,85 +5645,138 @@ SDValue TargetLowering::negateExpression(SDValue Op, SelectionDAG &DAG, V.changeSign(); Ops.push_back(DAG.getConstantFP(V, DL, C.getValueType())); } + Cost = NegatibleCost::Neutral; return DAG.getBuildVector(VT, DL, Ops); } case ISD::FADD: { + if (!Options.NoSignedZerosFPMath && !Flags.hasNoSignedZeros()) + break; + + // After operation legalization, it might not be legal to create new FSUBs. + if (LegalOps && !isOperationLegalOrCustom(ISD::FSUB, VT)) + break; SDValue X = Op.getOperand(0), Y = Op.getOperand(1); - assert((DAG.getTarget().Options.NoSignedZerosFPMath || - Flags.hasNoSignedZeros()) && - "Expected NSZ fp-flag"); // fold (fneg (fadd X, Y)) -> (fsub (fneg X), Y) - NegatibleCost CostX = getNegatibleCost(X, DAG, LegalOps, OptForSize, Depth); - if (CostX != NegatibleCost::Expensive) - return DAG.getNode(ISD::FSUB, DL, VT, - negateExpression(X, DAG, LegalOps, OptForSize, Depth), - Y, Flags); - + NegatibleCost CostX = NegatibleCost::Expensive; + SDValue NegX = + getNegatedExpression(X, DAG, LegalOps, OptForSize, CostX, Depth); // fold (fneg (fadd X, Y)) -> (fsub (fneg Y), X) - return DAG.getNode(ISD::FSUB, DL, VT, - negateExpression(Y, DAG, LegalOps, OptForSize, Depth), X, - Flags); + NegatibleCost CostY = NegatibleCost::Expensive; + SDValue NegY = + getNegatedExpression(Y, DAG, LegalOps, OptForSize, CostY, Depth); + + // Negate the X if its cost is less or equal than Y. + if (NegX && (CostX <= CostY)) { + Cost = CostX; + return DAG.getNode(ISD::FSUB, DL, VT, NegX, Y, Flags); + } + + // Negate the Y if it is not expensive. + if (NegY) { + Cost = CostY; + return DAG.getNode(ISD::FSUB, DL, VT, NegY, X, Flags); + } + break; } case ISD::FSUB: { + // We can't turn -(A-B) into B-A when we honor signed zeros. + if (!Options.NoSignedZerosFPMath && !Flags.hasNoSignedZeros()) + break; + SDValue X = Op.getOperand(0), Y = Op.getOperand(1); // fold (fneg (fsub 0, Y)) -> Y if (ConstantFPSDNode *C = isConstOrConstSplatFP(X, /*AllowUndefs*/ true)) - if (C->isZero()) + if (C->isZero()) { + Cost = NegatibleCost::Cheaper; return Y; + } // fold (fneg (fsub X, Y)) -> (fsub Y, X) + Cost = NegatibleCost::Neutral; return DAG.getNode(ISD::FSUB, DL, VT, Y, X, Flags); } case ISD::FMUL: case ISD::FDIV: { SDValue X = Op.getOperand(0), Y = Op.getOperand(1); - // fold (fneg (fmul X, Y)) -> (fmul (fneg X), Y) - NegatibleCost CostX = getNegatibleCost(X, DAG, LegalOps, OptForSize, Depth); - if (CostX != NegatibleCost::Expensive) - return DAG.getNode(Opcode, DL, VT, - negateExpression(X, DAG, LegalOps, OptForSize, Depth), - Y, Flags); + // fold (fneg (fmul X, Y)) -> (fmul (fneg X), Y) + NegatibleCost CostX = NegatibleCost::Expensive; + SDValue NegX = + getNegatedExpression(X, DAG, LegalOps, OptForSize, CostX, Depth); // fold (fneg (fmul X, Y)) -> (fmul X, (fneg Y)) - return DAG.getNode(Opcode, DL, VT, X, - negateExpression(Y, DAG, LegalOps, OptForSize, Depth), - Flags); + NegatibleCost CostY = NegatibleCost::Expensive; + SDValue NegY = + getNegatedExpression(Y, DAG, LegalOps, OptForSize, CostY, Depth); + + // Negate the X if its cost is less or equal than Y. + if (NegX && (CostX <= CostY)) { + Cost = CostX; + return DAG.getNode(Opcode, DL, VT, NegX, Y, Flags); + } + + // Ignore X * 2.0 because that is expected to be canonicalized to X + X. + if (auto *C = isConstOrConstSplatFP(Op.getOperand(1))) + if (C->isExactlyValue(2.0) && Op.getOpcode() == ISD::FMUL) + break; + + // Negate the Y if it is not expensive. + if (NegY) { + Cost = CostY; + return DAG.getNode(Opcode, DL, VT, X, NegY, Flags); + } + break; } case ISD::FMA: case ISD::FMAD: { - assert((DAG.getTarget().Options.NoSignedZerosFPMath || - Flags.hasNoSignedZeros()) && - "Expected NSZ fp-flag"); + if (!Options.NoSignedZerosFPMath && !Flags.hasNoSignedZeros()) + break; SDValue X = Op.getOperand(0), Y = Op.getOperand(1), Z = Op.getOperand(2); - SDValue NegZ = negateExpression(Z, DAG, LegalOps, OptForSize, Depth); - NegatibleCost CostX = getNegatibleCost(X, DAG, LegalOps, OptForSize, Depth); - NegatibleCost CostY = getNegatibleCost(Y, DAG, LegalOps, OptForSize, Depth); - if (CostX <= CostY) { - // fold (fneg (fma X, Y, Z)) -> (fma (fneg X), Y, (fneg Z)) - SDValue NegX = negateExpression(X, DAG, LegalOps, OptForSize, Depth); + NegatibleCost CostZ = NegatibleCost::Expensive; + SDValue NegZ = + getNegatedExpression(Z, DAG, LegalOps, OptForSize, CostZ, Depth); + // Give up if fail to negate the Z. + if (!NegZ) + break; + + // fold (fneg (fma X, Y, Z)) -> (fma (fneg X), Y, (fneg Z)) + NegatibleCost CostX = NegatibleCost::Expensive; + SDValue NegX = + getNegatedExpression(X, DAG, LegalOps, OptForSize, CostX, Depth); + // fold (fneg (fma X, Y, Z)) -> (fma X, (fneg Y), (fneg Z)) + NegatibleCost CostY = NegatibleCost::Expensive; + SDValue NegY = + getNegatedExpression(Y, DAG, LegalOps, OptForSize, CostY, Depth); + + // Negate the X if its cost is less or equal than Y. + if (NegX && (CostX <= CostY)) { + Cost = std::min(CostX, CostZ); return DAG.getNode(Opcode, DL, VT, NegX, Y, NegZ, Flags); } - // fold (fneg (fma X, Y, Z)) -> (fma X, (fneg Y), (fneg Z)) - SDValue NegY = negateExpression(Y, DAG, LegalOps, OptForSize, Depth); - return DAG.getNode(Opcode, DL, VT, X, NegY, NegZ, Flags); + // Negate the Y if it is not expensive. + if (NegY) { + Cost = std::min(CostY, CostZ); + return DAG.getNode(Opcode, DL, VT, X, NegY, NegZ, Flags); + } + break; } case ISD::FP_EXTEND: case ISD::FSIN: - return DAG.getNode( - Opcode, DL, VT, - negateExpression(Op.getOperand(0), DAG, LegalOps, OptForSize, Depth)); + if (SDValue NegV = getNegatedExpression(Op.getOperand(0), DAG, LegalOps, + OptForSize, Cost, Depth)) + return DAG.getNode(Opcode, DL, VT, NegV); + break; case ISD::FP_ROUND: - return DAG.getNode( - ISD::FP_ROUND, DL, VT, - negateExpression(Op.getOperand(0), DAG, LegalOps, OptForSize, Depth), - Op.getOperand(1)); + if (SDValue NegV = getNegatedExpression(Op.getOperand(0), DAG, LegalOps, + OptForSize, Cost, Depth)) + return DAG.getNode(ISD::FP_ROUND, DL, VT, NegV, Op.getOperand(1)); + break; } - llvm_unreachable("Unknown code"); + return SDValue(); } //===----------------------------------------------------------------------===// diff --git a/lib/Target/AMDGPU/AMDGPUISelLowering.cpp b/lib/Target/AMDGPU/AMDGPUISelLowering.cpp index b453098b682..1edc42adf9d 100644 --- a/lib/Target/AMDGPU/AMDGPUISelLowering.cpp +++ b/lib/Target/AMDGPU/AMDGPUISelLowering.cpp @@ -811,24 +811,24 @@ bool AMDGPUTargetLowering::isSDNodeAlwaysUniform(const SDNode * N) const { } } -TargetLowering::NegatibleCost -AMDGPUTargetLowering::getNegatibleCost(SDValue Op, SelectionDAG &DAG, - bool LegalOperations, bool ForCodeSize, - unsigned Depth) const { +SDValue AMDGPUTargetLowering::getNegatedExpression( + SDValue Op, SelectionDAG &DAG, bool LegalOperations, bool ForCodeSize, + NegatibleCost &Cost, unsigned Depth) const { + switch (Op.getOpcode()) { case ISD::FMA: case ISD::FMAD: { // Negating a fma is not free if it has users without source mods. if (!allUsesHaveSourceMods(Op.getNode())) - return NegatibleCost::Expensive; + return SDValue(); break; } default: break; } - return TargetLowering::getNegatibleCost(Op, DAG, LegalOperations, ForCodeSize, - Depth); + return TargetLowering::getNegatedExpression(Op, DAG, LegalOperations, + ForCodeSize, Cost, Depth); } //===---------------------------------------------------------------------===// diff --git a/lib/Target/AMDGPU/AMDGPUISelLowering.h b/lib/Target/AMDGPU/AMDGPUISelLowering.h index 137a18968d5..3934c7591a1 100644 --- a/lib/Target/AMDGPU/AMDGPUISelLowering.h +++ b/lib/Target/AMDGPU/AMDGPUISelLowering.h @@ -170,9 +170,10 @@ public: bool isZExtFree(EVT Src, EVT Dest) const override; bool isZExtFree(SDValue Val, EVT VT2) const override; - NegatibleCost getNegatibleCost(SDValue Op, SelectionDAG &DAG, - bool LegalOperations, bool ForCodeSize, - unsigned Depth) const override; + SDValue getNegatedExpression(SDValue Op, SelectionDAG &DAG, + bool LegalOperations, bool ForCodeSize, + NegatibleCost &Cost, + unsigned Depth) const override; bool isNarrowingProfitable(EVT VT1, EVT VT2) const override; diff --git a/lib/Target/X86/X86ISelLowering.cpp b/lib/Target/X86/X86ISelLowering.cpp index 08f455b8bf2..f4a88de7a1d 100644 --- a/lib/Target/X86/X86ISelLowering.cpp +++ b/lib/Target/X86/X86ISelLowering.cpp @@ -44067,60 +44067,16 @@ static SDValue combineFneg(SDNode *N, SelectionDAG &DAG, return SDValue(); } -TargetLowering::NegatibleCost -X86TargetLowering::getNegatibleCost(SDValue Op, SelectionDAG &DAG, - bool LegalOperations, bool ForCodeSize, - unsigned Depth) const { +SDValue X86TargetLowering::getNegatedExpression(SDValue Op, SelectionDAG &DAG, + bool LegalOperations, + bool ForCodeSize, + NegatibleCost &Cost, + unsigned Depth) const { // fneg patterns are removable even if they have multiple uses. - if (isFNEG(DAG, Op.getNode(), Depth)) - return NegatibleCost::Cheaper; - - // Don't recurse exponentially. - if (Depth > SelectionDAG::MaxRecursionDepth) - return NegatibleCost::Expensive; - - EVT VT = Op.getValueType(); - EVT SVT = VT.getScalarType(); - switch (Op.getOpcode()) { - case ISD::FMA: - case X86ISD::FMSUB: - case X86ISD::FNMADD: - case X86ISD::FNMSUB: - case X86ISD::FMADD_RND: - case X86ISD::FMSUB_RND: - case X86ISD::FNMADD_RND: - case X86ISD::FNMSUB_RND: { - if (!Op.hasOneUse() || !Subtarget.hasAnyFMA() || !isTypeLegal(VT) || - !(SVT == MVT::f32 || SVT == MVT::f64) || - !isOperationLegal(ISD::FMA, VT)) - break; - - // This is always negatible for free but we might be able to remove some - // extra operand negations as well. - for (int i = 0; i != 3; ++i) { - NegatibleCost V = getNegatibleCost(Op.getOperand(i), DAG, LegalOperations, - ForCodeSize, Depth + 1); - if (V == NegatibleCost::Cheaper) - return V; - } - return NegatibleCost::Neutral; - } - case X86ISD::FRCP: - return getNegatibleCost(Op.getOperand(0), DAG, LegalOperations, ForCodeSize, - Depth + 1); - } - - return TargetLowering::getNegatibleCost(Op, DAG, LegalOperations, ForCodeSize, - Depth); -} - -SDValue X86TargetLowering::negateExpression(SDValue Op, SelectionDAG &DAG, - bool LegalOperations, - bool ForCodeSize, - unsigned Depth) const { - // fneg patterns are removable even if they have multiple uses. - if (SDValue Arg = isFNEG(DAG, Op.getNode(), Depth)) + if (SDValue Arg = isFNEG(DAG, Op.getNode(), Depth)) { + Cost = NegatibleCost::Cheaper; return DAG.getBitcast(Op.getValueType(), Arg); + } EVT VT = Op.getValueType(); EVT SVT = VT.getScalarType(); @@ -44151,6 +44107,9 @@ SDValue X86TargetLowering::negateExpression(SDValue Op, SelectionDAG &DAG, bool NegC = !!NewOps[2]; unsigned NewOpc = negateFMAOpcode(Opc, NegA != NegB, NegC, true); + Cost = (NegA || NegB || NegC) ? NegatibleCost::Cheaper + : NegatibleCost::Neutral; + // Fill in the non-negated ops with the original values. for (int i = 0, e = Op.getNumOperands(); i != e; ++i) if (!NewOps[i]) @@ -44158,14 +44117,15 @@ SDValue X86TargetLowering::negateExpression(SDValue Op, SelectionDAG &DAG, return DAG.getNode(NewOpc, SDLoc(Op), VT, NewOps); } case X86ISD::FRCP: - return DAG.getNode(Opc, SDLoc(Op), VT, - negateExpression(Op.getOperand(0), DAG, LegalOperations, - ForCodeSize, Depth + 1)); + if (SDValue NegOp0 = + getNegatedExpression(Op.getOperand(0), DAG, LegalOperations, + ForCodeSize, Cost, Depth + 1)) + return DAG.getNode(Opc, SDLoc(Op), VT, NegOp0); break; } - return TargetLowering::negateExpression(Op, DAG, LegalOperations, ForCodeSize, - Depth); + return TargetLowering::getNegatedExpression(Op, DAG, LegalOperations, + ForCodeSize, Cost, Depth); } static SDValue lowerX86FPLogicOp(SDNode *N, SelectionDAG &DAG, diff --git a/lib/Target/X86/X86ISelLowering.h b/lib/Target/X86/X86ISelLowering.h index 06237208e81..1c9042c2ac3 100644 --- a/lib/Target/X86/X86ISelLowering.h +++ b/lib/Target/X86/X86ISelLowering.h @@ -935,17 +935,13 @@ namespace llvm { /// and some i16 instructions are slow. bool IsDesirableToPromoteOp(SDValue Op, EVT &PVT) const override; - /// Returns whether computing the negated form of the specified expression - /// is more expensive, the same cost or cheaper. - NegatibleCost getNegatibleCost(SDValue Op, SelectionDAG &DAG, - bool LegalOperations, bool ForCodeSize, - unsigned Depth) const override; - - /// If getNegatibleCost returns Neutral/Cheaper, return the newly negated - /// expression. - SDValue negateExpression(SDValue Op, SelectionDAG &DAG, - bool LegalOperations, bool ForCodeSize, - unsigned Depth) const override; + /// Return the newly negated expression if the cost is not expensive and + /// set the cost in \p Cost to indicate that if it is cheaper or neutral to + /// do the negation. + SDValue getNegatedExpression(SDValue Op, SelectionDAG &DAG, + bool LegalOperations, bool ForCodeSize, + NegatibleCost &Cost, + unsigned Depth) const override; MachineBasicBlock * EmitInstrWithCustomInserter(MachineInstr &MI, diff --git a/test/CodeGen/X86/neg_fp.ll b/test/CodeGen/X86/neg_fp.ll index 3c04aafcea4..e07157c952f 100644 --- a/test/CodeGen/X86/neg_fp.ll +++ b/test/CodeGen/X86/neg_fp.ll @@ -54,3 +54,30 @@ define double @negation_propagation(double* %arg, double %arg1, double %arg2) no %t18 = fadd double %t16, %t7 ret double %t18 } + +; This would crash because the negated expression for %sub4 +; creates a new use of %sub1 and that alters the negated cost + +define float @fdiv_extra_use_changes_cost(float %a0, float %a1, float %a2) nounwind { +; CHECK-LABEL: fdiv_extra_use_changes_cost: +; CHECK: # %bb.0: +; CHECK-NEXT: pushl %eax +; CHECK-NEXT: movss {{.*#+}} xmm0 = mem[0],zero,zero,zero +; CHECK-NEXT: movss {{.*#+}} xmm1 = mem[0],zero,zero,zero +; CHECK-NEXT: subss {{[0-9]+}}(%esp), %xmm1 +; CHECK-NEXT: movaps %xmm1, %xmm2 +; CHECK-NEXT: mulss %xmm0, %xmm2 +; CHECK-NEXT: subss %xmm1, %xmm0 +; CHECK-NEXT: divss %xmm2, %xmm0 +; CHECK-NEXT: movss %xmm0, (%esp) +; CHECK-NEXT: flds (%esp) +; CHECK-NEXT: popl %eax +; CHECK-NEXT: retl + %sub1 = fsub fast float %a0, %a1 + %mul2 = fmul fast float %sub1, %a2 + %neg = fneg fast float %a0 + %add3 = fadd fast float %a1, %neg + %sub4 = fadd fast float %add3, %a2 + %div5 = fdiv fast float %sub4, %mul2 + ret float %div5 +}