Rough out InstCombine::visitFNeg(...)

[cameron.mcinally]

Summary

Start by pulling out a little chunk of code that FNeg and FSub will share. More to follow...

[lebedev.ri]

llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp

1828–1829 ↗

(On Diff #198711)

I'm not confident this one-use check should be here.
We start with two ops - negation and some inner operand.
We will drop the negation, replace with with a single op we build.
Therefore there is no instruction count explosion.
And less instructions should be pretty much always better.

Though i see that this is only moving it around.

The performance concern (the inverse transform,
X * (-C) -> -(X * C) if (X * C) already exists)
should be a separate fold in dagcombine..

[cameron.mcinally]

llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp

1828–1829 ↗

(On Diff #198711)

I'm not confident this one-use check should be here.
We start with two ops - negation and some inner operand.
We will drop the negation, replace with with a single op we build.
Therefore there is no instruction count explosion.
And less instructions should be pretty much always better.

Depends on the architecture really. Worst case, we could end up with a NEG and a MUL, or alternatively 2 MUL's. If a MUL is slower than an XOR (the NEG), then the check would make sense.

The performance concern (the inverse transform,
X * (-C) -> -(X * C) if (X * C) already exists)
should be a separate fold in dagcombine..

That's a good peep. Should probably be handled in a separate Diff though. I'm trying not to modify code here, just replicate FSub xforms for the new FNeg.

[spatel]

llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp

1828–1829 ↗

(On Diff #198711)

That was D50417.

If there are multiple uses, the 2 potential forms would look something like this:

T = fdiv X, C
R = fneg T

vs.

T = fdiv X, C
R = fdiv X,-C

So correct, no difference in instruction count. And I agree that this isn't our usual trade-off in IR/analysis. But I do wonder if the fneg allows for better reassociation opportunities. Not sure how to check that though.

I also agree on the codegen/perf concern, but that would have to be added first to the backend IMO to avoid (the theoretical, but potentially large) regression, so I went with the conservative quick fix.

I don't think we can get away with an SDAG solution. If those fdivs end up in different basic blocks somehow, we need a global analysis to know the siblings exist.

[cameron.mcinally]

Rename CreateWithCopiedFlags(...) argument, since it is no longer strictly a BinaryOperator.

[cameron.mcinally]

Rebase patch. It's a little more palatable after merging D61784.

[cameron.mcinally]

Ping.

Are we okay with addressing the concerns @lebedev.ri brought up in a different patch? I could file a bug if wanted.

[spatel]

In D61693#1508481, @cameron.mcinally wrote:

Ping.

LGTM

Are we okay with addressing the concerns @lebedev.ri brought up in a different patch? I could file a bug if wanted.

It's definitely a different patch (if we can actually do anything about it). Adding details from the discussion here to the existing code comment would be good as a separate NFC too.

llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp

1824 ↗

(On Diff #199068)

Would be nice to have a documentation comment for this function that says something like:
This eliminates floating-point negation in either 'fneg' or 'fsub' form by combining into a constant operand.

[cameron.mcinally]

Bah, I forgot to amend my commit message before committing. For posterity's sake, it should have read:

[InstCombine] Create foldFNegIntoConstant(...) helper for FNeg

Break out a helper function, namely foldFNegIntoConstant(...), which performs transforms common between visitFNeg(...) and visitFSub(...).

Differential Revision: https://reviews.llvm.org/D61693

llvm/trunk/include/llvm/IR/InstrTypes.h

#define HANDLE_BINARY_INST(N, OPC, CLASS) \
   static BinaryOperator *Create##OPC(Value *V1, Value *V2, \
                                      const Twine &Name, Instruction *I) {\
     return Create(Instruction::OPC, V1, V2, Name, I);\
   }
 #include "llvm/IR/Instruction.def"
 
   static BinaryOperator *CreateWithCopiedFlags(BinaryOps Opc,
                                                Value *V1, Value *V2,
-                                               BinaryOperator *CopyBO,
+                                               Instruction *CopyO,
                                                const Twine &Name = "") {
     BinaryOperator *BO = Create(Opc, V1, V2, Name);
-    BO->copyIRFlags(CopyBO);
+    BO->copyIRFlags(CopyO);
     return BO;
   }
 
   static BinaryOperator *CreateFAddFMF(Value *V1, Value *V2,
-                                       BinaryOperator *FMFSource,
+                                       Instruction *FMFSource,
                                        const Twine &Name = "") {
     return CreateWithCopiedFlags(Instruction::FAdd, V1, V2, FMFSource, Name);
   }
   static BinaryOperator *CreateFSubFMF(Value *V1, Value *V2,
-                                       BinaryOperator *FMFSource,
+                                       Instruction *FMFSource,
                                        const Twine &Name = "") {
     return CreateWithCopiedFlags(Instruction::FSub, V1, V2, FMFSource, Name);
   }
   static BinaryOperator *CreateFMulFMF(Value *V1, Value *V2,
-                                       BinaryOperator *FMFSource,
+                                       Instruction *FMFSource,
                                        const Twine &Name = "") {
     return CreateWithCopiedFlags(Instruction::FMul, V1, V2, FMFSource, Name);
   }
   static BinaryOperator *CreateFDivFMF(Value *V1, Value *V2,
-                                       BinaryOperator *FMFSource,
+                                       Instruction *FMFSource,
                                        const Twine &Name = "") {
     return CreateWithCopiedFlags(Instruction::FDiv, V1, V2, FMFSource, Name);
   }
   static BinaryOperator *CreateFRemFMF(Value *V1, Value *V2,
-                                       BinaryOperator *FMFSource,
+                                       Instruction *FMFSource,
                                        const Twine &Name = "") {
     return CreateWithCopiedFlags(Instruction::FRem, V1, V2, FMFSource, Name);
   }
-  static BinaryOperator *CreateFNegFMF(Value *Op, BinaryOperator *FMFSource,
+  static BinaryOperator *CreateFNegFMF(Value *Op, Instruction *FMFSource,
                                        const Twine &Name = "") {
     Value *Zero = ConstantFP::getNegativeZero(Op->getType());
     return CreateWithCopiedFlags(Instruction::FSub, Zero, Op, FMFSource);
   }
 
   static BinaryOperator *CreateNSW(BinaryOps Opc, Value *V1, Value *V2,
                                    const Twine &Name = "") {
     BinaryOperator *BO = Create(Opc, V1, V2, Name);

llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp

Instruction *InstCombiner::visitSub(BinaryOperator &I) {
   if (!I.hasNoUnsignedWrap() && willNotOverflowUnsignedSub(Op0, Op1, I)) {
     Changed = true;
     I.setHasNoUnsignedWrap(true);
   }
 
   return Changed ? &I : nullptr;
 }
 
+/// This eliminates floating-point negation in either 'fneg(X)' or
+/// 'fsub(-0.0, X)' form by combining into a constant operand.
+static Instruction *foldFNegIntoConstant(Instruction &I) {
+  Value *X;
+  Constant *C;
+
+  // Fold negation into constant operand. This is limited with one-use because
+  // fneg is assumed better for analysis and cheaper in codegen than fmul/fdiv.
+  // -(X * C) --> X * (-C)
+  if (match(&I, m_FNeg(m_OneUse(m_FMul(m_Value(X), m_Constant(C))))))
+    return BinaryOperator::CreateFMulFMF(X, ConstantExpr::getFNeg(C), &I);
+  // -(X / C) --> X / (-C)
+  if (match(&I, m_FNeg(m_OneUse(m_FDiv(m_Value(X), m_Constant(C))))))
+    return BinaryOperator::CreateFDivFMF(X, ConstantExpr::getFNeg(C), &I);
+  // -(C / X) --> (-C) / X
+  if (match(&I, m_FNeg(m_OneUse(m_FDiv(m_Constant(C), m_Value(X))))))
+    return BinaryOperator::CreateFDivFMF(ConstantExpr::getFNeg(C), X, &I);
+
+  return nullptr;
+}
+
 Instruction *InstCombiner::visitFNeg(UnaryOperator &I) {
   if (Value *V = SimplifyFNegInst(I.getOperand(0), I.getFastMathFlags(),
                                   SQ.getWithInstruction(&I)))
     return replaceInstUsesWith(I, V);
 
+  if (Instruction *X = foldFNegIntoConstant(I))
+    return X;
+
   return nullptr;
 }
 
-
 Instruction *InstCombiner::visitFSub(BinaryOperator &I) {
   if (Value *V = SimplifyFSubInst(I.getOperand(0), I.getOperand(1),
                                   I.getFastMathFlags(),
                                   SQ.getWithInstruction(&I)))
     return replaceInstUsesWith(I, V);
 
   if (Instruction *X = foldVectorBinop(I))
     return X;
 
   // Subtraction from -0.0 is the canonical form of fneg.
   // fsub nsz 0, X ==> fsub nsz -0.0, X
   Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
   if (I.hasNoSignedZeros() && match(Op0, m_PosZeroFP()))
     return BinaryOperator::CreateFNegFMF(Op1, &I);
 
+  if (Instruction *X = foldFNegIntoConstant(I))
+    return X;
+
   Value *X, *Y;
   Constant *C;
 
-  // Fold negation into constant operand. This is limited with one-use because
-  // fneg is assumed better for analysis and cheaper in codegen than fmul/fdiv.
-  // -(X * C) --> X * (-C)
-  if (match(&I, m_FNeg(m_OneUse(m_FMul(m_Value(X), m_Constant(C))))))
-    return BinaryOperator::CreateFMulFMF(X, ConstantExpr::getFNeg(C), &I);
-  // -(X / C) --> X / (-C)
-  if (match(&I, m_FNeg(m_OneUse(m_FDiv(m_Value(X), m_Constant(C))))))
-    return BinaryOperator::CreateFDivFMF(X, ConstantExpr::getFNeg(C), &I);
-  // -(C / X) --> (-C) / X
-  if (match(&I, m_FNeg(m_OneUse(m_FDiv(m_Constant(C), m_Value(X))))))
-    return BinaryOperator::CreateFDivFMF(ConstantExpr::getFNeg(C), X, &I);
-
   // If Op0 is not -0.0 or we can ignore -0.0: Z - (X - Y) --> Z + (Y - X)
   // Canonicalize to fadd to make analysis easier.
   // This can also help codegen because fadd is commutative.
   // Note that if this fsub was really an fneg, the fadd with -0.0 will get
   // killed later. We still limit that particular transform with 'hasOneUse'
   // because an fneg is assumed better/cheaper than a generic fsub.
   if (I.hasNoSignedZeros() || CannotBeNegativeZero(Op0, SQ.TLI)) {
     if (match(Op1, m_OneUse(m_FSub(m_Value(X), m_Value(Y))))) {

llvm/trunk/test/Transforms/InstCombine/fneg.ll

 ;
   %m = fmul float %x, 42.0
   %r = fsub float -0.0, %m
   ret float %r
 }
 
 define float @fmul_fneg(float %x) {
 ; CHECK-LABEL: @fmul_fneg(
-; CHECK-NEXT:    [[M:%.*]] = fmul float [[X:%.*]], 4.200000e+01
-; CHECK-NEXT:    [[R:%.*]] = fneg float [[M]]
+; CHECK-NEXT:    [[R:%.*]] = fmul float [[X:%.*]], -4.200000e+01
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %m = fmul float %x, 42.0
   %r = fneg float %m
   ret float %r
 }
 
 ; Fast math is not required, but it should be propagated.
 
 define float @fmul_fsub_fmf(float %x) {
 ; CHECK-LABEL: @fmul_fsub_fmf(
 ; CHECK-NEXT:    [[R:%.*]] = fmul reassoc nsz float [[X:%.*]], -4.200000e+01
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %m = fmul float %x, 42.0
   %r = fsub reassoc nsz float -0.0, %m
   ret float %r
 }
 
 define float @fmul_fneg_fmf(float %x) {
 ; CHECK-LABEL: @fmul_fneg_fmf(
-; CHECK-NEXT:    [[M:%.*]] = fmul float [[X:%.*]], 4.200000e+01
-; CHECK-NEXT:    [[R:%.*]] = fneg reassoc nsz float [[M]]
+; CHECK-NEXT:    [[R:%.*]] = fmul reassoc nsz float [[X:%.*]], -4.200000e+01
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %m = fmul float %x, 42.0
   %r = fneg reassoc nsz float %m
   ret float %r
 }
 
 ; Extra use prevents the fold. We don't want to replace the fneg with an fmul.

 ;
   %m = fmul <4 x double> %x, <double 42.0, double 0x7FF80000000000000, double 0x7FF0000000000000, double undef>
   %r = fsub <4 x double> <double -0.0, double -0.0, double -0.0, double -0.0>, %m
   ret <4 x double> %r
 }
 
 define <4 x double> @fmul_fneg_vec(<4 x double> %x) {
 ; CHECK-LABEL: @fmul_fneg_vec(
-; CHECK-NEXT:    [[M:%.*]] = fmul <4 x double> [[X:%.*]], <double 4.200000e+01, double 0xFF80000000000000, double 0x7FF0000000000000, double undef>
-; CHECK-NEXT:    [[R:%.*]] = fneg <4 x double> [[M]]
+; CHECK-NEXT:    [[R:%.*]] = fmul <4 x double> [[X:%.*]], <double -4.200000e+01, double 0x7F80000000000000, double 0xFFF0000000000000, double undef>
 ; CHECK-NEXT:    ret <4 x double> [[R]]
 ;
   %m = fmul <4 x double> %x, <double 42.0, double 0x7FF80000000000000, double 0x7FF0000000000000, double undef>
   %r = fneg <4 x double> %m
   ret <4 x double> %r
 }
 
 ; -(X / C) --> X / (-C)
 
 define float @fdiv_op1_constant_fsub(float %x) {
 ; CHECK-LABEL: @fdiv_op1_constant_fsub(
 ; CHECK-NEXT:    [[R:%.*]] = fdiv float [[X:%.*]], 4.200000e+01
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %d = fdiv float %x, -42.0
   %r = fsub float -0.0, %d
   ret float %r
 }
 
 define float @fdiv_op1_constant_fneg(float %x) {
 ; CHECK-LABEL: @fdiv_op1_constant_fneg(
-; CHECK-NEXT:    [[D:%.*]] = fdiv float [[X:%.*]], -4.200000e+01
-; CHECK-NEXT:    [[R:%.*]] = fneg float [[D]]
+; CHECK-NEXT:    [[R:%.*]] = fdiv float [[X:%.*]], 4.200000e+01
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %d = fdiv float %x, -42.0
   %r = fneg float %d
   ret float %r
 }
 
 ; Fast math is not required, but it should be propagated.
 
 define float @fdiv_op1_constant_fsub_fmf(float %x) {
 ; CHECK-LABEL: @fdiv_op1_constant_fsub_fmf(
 ; CHECK-NEXT:    [[R:%.*]] = fdiv nnan float [[X:%.*]], 4.200000e+01
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %d = fdiv float %x, -42.0
   %r = fsub nnan float -0.0, %d
   ret float %r
 }
 
 define float @fdiv_op1_constant_fneg_fmf(float %x) {
 ; CHECK-LABEL: @fdiv_op1_constant_fneg_fmf(
-; CHECK-NEXT:    [[D:%.*]] = fdiv float [[X:%.*]], -4.200000e+01
-; CHECK-NEXT:    [[R:%.*]] = fneg nnan float [[D]]
+; CHECK-NEXT:    [[R:%.*]] = fdiv nnan float [[X:%.*]], 4.200000e+01
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %d = fdiv float %x, -42.0
   %r = fneg nnan float %d
   ret float %r
 }
 
 ; Extra use prevents the fold. We don't want to replace the fneg with an fdiv.

 ;
   %d = fdiv <4 x double> %x, <double -42.0, double 0xFFF800000ABCD000, double 0xFFF0000000000000, double undef>
   %r = fsub <4 x double> <double -0.0, double -0.0, double -0.0, double -0.0>, %d
   ret <4 x double> %r
 }
 
 define <4 x double> @fdiv_op1_constant_fneg_vec(<4 x double> %x) {
 ; CHECK-LABEL: @fdiv_op1_constant_fneg_vec(
-; CHECK-NEXT:    [[D:%.*]] = fdiv <4 x double> [[X:%.*]], <double -4.200000e+01, double 0xFFF800000ABCD000, double 0xFFF0000000000000, double undef>
-; CHECK-NEXT:    [[R:%.*]] = fneg <4 x double> [[D]]
+; CHECK-NEXT:    [[R:%.*]] = fdiv <4 x double> [[X:%.*]], <double 4.200000e+01, double 0x7FF800000ABCD000, double 0x7FF0000000000000, double undef>
 ; CHECK-NEXT:    ret <4 x double> [[R]]
 ;
   %d = fdiv <4 x double> %x, <double -42.0, double 0xFFF800000ABCD000, double 0xFFF0000000000000, double undef>
   %r = fneg <4 x double> %d
   ret <4 x double> %r
 }
 
 ; -(C / X) --> (-C) / X
 
 define float @fdiv_op0_constant_fsub(float %x) {
 ; CHECK-LABEL: @fdiv_op0_constant_fsub(
 ; CHECK-NEXT:    [[R:%.*]] = fdiv float -4.200000e+01, [[X:%.*]]
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %d = fdiv float 42.0, %x
   %r = fsub float -0.0, %d
   ret float %r
 }
 
 define float @fdiv_op0_constant_fneg(float %x) {
 ; CHECK-LABEL: @fdiv_op0_constant_fneg(
-; CHECK-NEXT:    [[D:%.*]] = fdiv float 4.200000e+01, [[X:%.*]]
-; CHECK-NEXT:    [[R:%.*]] = fneg float [[D]]
+; CHECK-NEXT:    [[R:%.*]] = fdiv float -4.200000e+01, [[X:%.*]]
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %d = fdiv float 42.0, %x
   %r = fneg float %d
   ret float %r
 }
 
 ; Fast math is not required, but it should be propagated.
 
 define float @fdiv_op0_constant_fsub_fmf(float %x) {
 ; CHECK-LABEL: @fdiv_op0_constant_fsub_fmf(
 ; CHECK-NEXT:    [[R:%.*]] = fdiv fast float -4.200000e+01, [[X:%.*]]
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %d = fdiv float 42.0, %x
   %r = fsub fast float -0.0, %d
   ret float %r
 }
 
 define float @fdiv_op0_constant_fneg_fmf(float %x) {
 ; CHECK-LABEL: @fdiv_op0_constant_fneg_fmf(
-; CHECK-NEXT:    [[D:%.*]] = fdiv float 4.200000e+01, [[X:%.*]]
-; CHECK-NEXT:    [[R:%.*]] = fneg fast float [[D]]
+; CHECK-NEXT:    [[R:%.*]] = fdiv fast float -4.200000e+01, [[X:%.*]]
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %d = fdiv float 42.0, %x
   %r = fneg fast float %d
   ret float %r
 }
 
 ; Extra use prevents the fold. We don't want to replace the fneg with an fdiv.

 ;
   %d = fdiv <4 x double> <double -42.0, double 0x7FF80000000000000, double 0xFFF0000000000000, double undef>, %x
   %r = fsub <4 x double> <double -0.0, double -0.0, double -0.0, double -0.0>, %d
   ret <4 x double> %r
 }
 
 define <4 x double> @fdiv_op0_constant_fneg_vec(<4 x double> %x) {
 ; CHECK-LABEL: @fdiv_op0_constant_fneg_vec(
-; CHECK-NEXT:    [[D:%.*]] = fdiv <4 x double> <double -4.200000e+01, double 0xFF80000000000000, double 0xFFF0000000000000, double undef>, [[X:%.*]]
-; CHECK-NEXT:    [[R:%.*]] = fneg <4 x double> [[D]]
+; CHECK-NEXT:    [[R:%.*]] = fdiv <4 x double> <double 4.200000e+01, double 0x7F80000000000000, double 0x7FF0000000000000, double undef>, [[X:%.*]]
 ; CHECK-NEXT:    ret <4 x double> [[R]]
 ;
   %d = fdiv <4 x double> <double -42.0, double 0x7FF80000000000000, double 0xFFF0000000000000, double undef>, %x
   %r = fneg <4 x double> %d
   ret <4 x double> %r
 }
 
 ; Sink FP negation through a select: