[InstCombine] Use +0.0 instead of -0.0 as the FP identity for some folds

[david-arm]

Summary

In foldSelectIntoOp we sometimes transform a select of a fadd into a
fadd of a select, where we select between data and an identity value.
For both fadd and fsub the identity is always -0.0, but if the nsz
flag is set on the select instruction we can use +0.0 instead. Doing
so then triggers other optimisations, such as when folding the select
of masked load into a new masked load.

[spatel]

llvm/lib/IR/Constants.cpp

2851–2852

Remove the TODO comment.

2852–2853

This API is awkward. Should we change the constant's method to take a positive/negative param (to match the APFloat call within this call)?

ConstantFP::getZero(Type, bool Negative = false)

llvm/test/Transforms/InstCombine/select-binop-foldable-floating-point.ll

66

We can get a bit more coverage from this test by adding some extra FMF (nnan or ninf for example). That way, we'll verify that all flags are propagated as expected to the new select instruction.

This could also swap the select's true/false operands, so we're testing each of the changed code paths.

Add some FMF on the fadd too?

169

This pair of tests already transformed, but we dropped the FMF right? This will be easier to see if we pre-commit the tests with the baseline CHECK lines.

[dmgreen]

The Arm backend has patterns to turn the awkward fadd(select(.., -0.0)) back into a more natural predicated fadd. They will need to be made to work with both the forms, if this is changing.

[david-arm]

In D126774#3553210, @dmgreen wrote:

The Arm backend has patterns to turn the awkward fadd(select(.., -0.0)) back into a more natural predicated fadd. They will need to be made to work with both the forms, if this is changing.

That's a good point, although it's a shame that there isn't a single test in CodeGen/ARM that defends the DAG combine in PerformFAddVSelectCombine. So I'm not entirely sure what DAG combine I need for the IR cases that you're worried about. :(

[david-arm]

In D126774#3553210, @dmgreen wrote:

The Arm backend has patterns to turn the awkward fadd(select(.., -0.0)) back into a more natural predicated fadd. They will need to be made to work with both the forms, if this is changing.

Ah! My mistake - was looking in the wrong backend - should be CodeGen/Thumb2. Doh!

[dmgreen]

Yeah - it's handled via tablegen patterns at the moment, from these:
https://github.com/llvm/llvm-project/blob/4fed5f174fa57c73907bb3344018a5f9c2bc2e68/llvm/lib/Target/ARM/ARMInstrMVE.td#L3774

And comes up from any float reduction I think, due to the way we specify the predicated reduction op in the loop:
https://godbolt.org/z/GjhhbKx9b

[david-arm]

• Added more pattern matches for MVE targets so we detect splats of +0.0 as well as -0.0.
• Added new ConstantFP::getZero interface.
• Added more flags to the tests for more coverage.

[david-arm]

llvm/lib/IR/Constants.cpp

2852–2853

Good suggestion @spatel !

[spatel]

In D126774#3563036, @david-arm wrote:

• Added more pattern matches for MVE targets so we detect splats of +0.0 as well as -0.0.

The backend code change and tests should be a separate patch/review that lands before the InstCombine change, so we don't regress those patterns.

[david-arm]

Hi @spatel, I've split the MVE changes out into D127275.

[spatel]

It would be good to refactor foldSelectIntoOp() with a lambda as a preliminary cleanup, so we don't have to duplicate the code.
But this is an improvement even without that, so LGTM.

[david-arm]

In D126774#3566251, @spatel wrote:

It would be good to refactor foldSelectIntoOp() with a lambda as a preliminary cleanup, so we don't have to duplicate the code.
But this is an improvement even without that, so LGTM.

I think cleaning this up makes sense. I'll merge this patch first, then create a follow-on NFC to refactor it.

llvm/include/llvm/IR/Constants.h

public:
   static Constant *get(Type *Ty, StringRef Str);
   static ConstantFP *get(LLVMContext &Context, const APFloat &V);
   static Constant *getNaN(Type *Ty, bool Negative = false,
                           uint64_t Payload = 0);
   static Constant *getQNaN(Type *Ty, bool Negative = false,
                            APInt *Payload = nullptr);
   static Constant *getSNaN(Type *Ty, bool Negative = false,
                            APInt *Payload = nullptr);
-  static Constant *getNegativeZero(Type *Ty);
+  static Constant *getZero(Type *Ty, bool Negative = false);
+  static Constant *getNegativeZero(Type *Ty) { return getZero(Ty, true); }
   static Constant *getInfinity(Type *Ty, bool Negative = false);
 
   /// Return true if Ty is big enough to represent V.
   static bool isValueValidForType(Type *Ty, const APFloat &V);
   inline const APFloat &getValueAPF() const { return Val; }
   inline const APFloat &getValue() const { return Val; }
 
   /// Return true if the value is positive or negative zero.
public:
   static Constant *getExactLogBase2(Constant *C);
 
   /// Return the identity constant for a binary opcode.
   /// The identity constant C is defined as X op C = X and C op X = X for every
   /// X when the binary operation is commutative. If the binop is not
   /// commutative, callers can acquire the operand 1 identity constant by
   /// setting AllowRHSConstant to true. For example, any shift has a zero
   /// identity constant for operand 1: X shift 0 = X.
+  /// If this is a fadd/fsub operation and we don't care about signed zeros,
+  /// then setting NSZ to true returns the identity +0.0 instead of -0.0.
   /// Return nullptr if the operator does not have an identity constant.
   static Constant *getBinOpIdentity(unsigned Opcode, Type *Ty,
-                                    bool AllowRHSConstant = false);
+                                    bool AllowRHSConstant = false,
+                                    bool NSZ = false);
 
   /// Return the absorbing element for the given binary
   /// operation, i.e. a constant C such that X op C = C and C op X = C for
   /// every X.  For example, this returns zero for integer multiplication.
   /// It returns null if the operator doesn't have an absorbing element.
   static Constant *getBinOpAbsorber(unsigned Opcode, Type *Ty);
 
   /// Transparently provide more efficient getOperand methods.

llvm/lib/IR/Constants.cpp

Constant *ConstantFP::getSNaN(Type *Ty, bool Negative, APInt *Payload) {
   Constant *C = get(Ty->getContext(), NaN);
 
   if (VectorType *VTy = dyn_cast<VectorType>(Ty))
     return ConstantVector::getSplat(VTy->getElementCount(), C);
 
   return C;
 }
 
-Constant *ConstantFP::getNegativeZero(Type *Ty) {
+Constant *ConstantFP::getZero(Type *Ty, bool Negative) {
   const fltSemantics &Semantics = Ty->getScalarType()->getFltSemantics();
-  APFloat NegZero = APFloat::getZero(Semantics, /*Negative=*/true);
+  APFloat NegZero = APFloat::getZero(Semantics, Negative);
   Constant *C = get(Ty->getContext(), NegZero);
 
   if (VectorType *VTy = dyn_cast<VectorType>(Ty))
     return ConstantVector::getSplat(VTy->getElementCount(), C);
 
   return C;
 }
 
-
 Constant *ConstantFP::getZeroValueForNegation(Type *Ty) {
   if (Ty->isFPOrFPVectorTy())
     return getNegativeZero(Ty);
 
   return Constant::getNullValue(Ty);
 }
 
 
if (!match(Elt, m_APInt(IVal)) || !IVal->isPowerOf2())
       return nullptr;
     Elts.push_back(ConstantInt::get(Ty->getScalarType(), IVal->logBase2()));
   }
 
   return ConstantVector::get(Elts);
 }
 
 Constant *ConstantExpr::getBinOpIdentity(unsigned Opcode, Type *Ty,
-                                         bool AllowRHSConstant) {
+                                         bool AllowRHSConstant, bool NSZ) {
   assert(Instruction::isBinaryOp(Opcode) && "Only binops allowed");
 
   // Commutative opcodes: it does not matter if AllowRHSConstant is set.
   if (Instruction::isCommutative(Opcode)) {
     switch (Opcode) {
       case Instruction::Add: // X + 0 = X
       case Instruction::Or:  // X | 0 = X
       case Instruction::Xor: // X ^ 0 = X
         return Constant::getNullValue(Ty);
       case Instruction::Mul: // X * 1 = X
         return ConstantInt::get(Ty, 1);
       case Instruction::And: // X & -1 = X
         return Constant::getAllOnesValue(Ty);
       case Instruction::FAdd: // X + -0.0 = X
-        // TODO: If the fadd has 'nsz', should we return +0.0?
-        return ConstantFP::getNegativeZero(Ty);
+        return ConstantFP::getZero(Ty, !NSZ);
       case Instruction::FMul: // X * 1.0 = X
         return ConstantFP::get(Ty, 1.0);
       default:
         llvm_unreachable("Every commutative binop has an identity constant");
     }
   }
 
   // Non-commutative opcodes: AllowRHSConstant must be set.
   if (!AllowRHSConstant)

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

if (TVI->hasOneUse() && !isa<Constant>(FalseVal)) {
         unsigned OpToFold = 0;
         if ((SFO & 1) && FalseVal == TVI->getOperand(0)) {
           OpToFold = 1;
         } else if ((SFO & 2) && FalseVal == TVI->getOperand(1)) {
           OpToFold = 2;
         }
 
         if (OpToFold) {
-          Constant *C = ConstantExpr::getBinOpIdentity(TVI->getOpcode(),
-                                                       TVI->getType(), true);
+          FastMathFlags FMF;
+          if (isa<FPMathOperator>(&SI))
+            FMF = SI.getFastMathFlags();
+          Constant *C = ConstantExpr::getBinOpIdentity(
+              TVI->getOpcode(), TVI->getType(), true, FMF.noSignedZeros());
           Value *OOp = TVI->getOperand(2-OpToFold);
           // Avoid creating select between 2 constants unless it's selecting
           // between 0, 1 and -1.
           const APInt *OOpC;
           bool OOpIsAPInt = match(OOp, m_APInt(OOpC));
           if (!isa<Constant>(OOp) ||
               (OOpIsAPInt && isSelect01(C->getUniqueInteger(), *OOpC))) {
             Value *NewSel = Builder.CreateSelect(SI.getCondition(), OOp, C);
+            if (isa<FPMathOperator>(&SI))
+              cast<Instruction>(NewSel)->setFastMathFlags(FMF);
             NewSel->takeName(TVI);
             BinaryOperator *BO = BinaryOperator::Create(TVI->getOpcode(),
                                                         FalseVal, NewSel);
             BO->copyIRFlags(TVI);
             return BO;
           }
         }
       }
     }
   }
 
   if (auto *FVI = dyn_cast<BinaryOperator>(FalseVal)) {
     if (FVI->hasOneUse() && !isa<Constant>(TrueVal)) {
       if (unsigned SFO = getSelectFoldableOperands(FVI)) {
         unsigned OpToFold = 0;
         if ((SFO & 1) && TrueVal == FVI->getOperand(0)) {
           OpToFold = 1;
         } else if ((SFO & 2) && TrueVal == FVI->getOperand(1)) {
           OpToFold = 2;
         }
 
         if (OpToFold) {
-          Constant *C = ConstantExpr::getBinOpIdentity(FVI->getOpcode(),
-                                                       FVI->getType(), true);
+          FastMathFlags FMF;
+          if (isa<FPMathOperator>(&SI))
+            FMF = SI.getFastMathFlags();
+          Constant *C = ConstantExpr::getBinOpIdentity(
+              FVI->getOpcode(), FVI->getType(), true, FMF.noSignedZeros());
           Value *OOp = FVI->getOperand(2-OpToFold);
           // Avoid creating select between 2 constants unless it's selecting
           // between 0, 1 and -1.
           const APInt *OOpC;
           bool OOpIsAPInt = match(OOp, m_APInt(OOpC));
           if (!isa<Constant>(OOp) ||
               (OOpIsAPInt && isSelect01(C->getUniqueInteger(), *OOpC))) {
             Value *NewSel = Builder.CreateSelect(SI.getCondition(), C, OOp);
+            if (isa<FPMathOperator>(&SI))
+              cast<Instruction>(NewSel)->setFastMathFlags(FMF);
             NewSel->takeName(FVI);
             BinaryOperator *BO = BinaryOperator::Create(FVI->getOpcode(),
                                                         TrueVal, NewSel);
             BO->copyIRFlags(FVI);
             return BO;
           }
         }
       }

llvm/test/Transforms/InstCombine/select-binop-foldable-floating-point.ll

 ; CHECK-NEXT:    [[D:%.*]] = fadd fast float [[C]], [[A:%.*]]
 ; CHECK-NEXT:    ret float [[D]]
 ;
   %C = fadd fast float %A, %B
   %D = select i1 %cond, float %A, float %C
   ret float %D
 }
 
+define <4 x float> @select_nsz_fadd_v4f32(<4 x i1> %cond, <4 x float> %A, <4 x float> %B) {
+; CHECK-LABEL: @select_nsz_fadd_v4f32(
+; CHECK-NEXT:    [[C:%.*]] = select nnan nsz <4 x i1> [[COND:%.*]], <4 x float> [[B:%.*]], <4 x float> zeroinitializer
+; CHECK-NEXT:    [[D:%.*]] = fadd nnan nsz <4 x float> [[C]], [[A:%.*]]
+; CHECK-NEXT:    ret <4 x float> [[D]]
+;
+  %C = fadd nsz nnan <4 x float> %A, %B
+  %D = select nsz nnan <4 x i1> %cond, <4 x float> %C, <4 x float> %A
+  ret <4 x float> %D
+}
+
+define <vscale x 4 x float> @select_nsz_fadd_nxv4f32(<vscale x 4 x i1> %cond, <vscale x 4 x float> %A, <vscale x 4 x float> %B) {
+; CHECK-LABEL: @select_nsz_fadd_nxv4f32(
+; CHECK-NEXT:    [[C:%.*]] = select nnan nsz <vscale x 4 x i1> [[COND:%.*]], <vscale x 4 x float> [[B:%.*]], <vscale x 4 x float> zeroinitializer
+; CHECK-NEXT:    [[D:%.*]] = fadd nnan nsz <vscale x 4 x float> [[C]], [[A:%.*]]
+; CHECK-NEXT:    ret <vscale x 4 x float> [[D]]
+;
+  %C = fadd nsz nnan <vscale x 4 x float> %A, %B
+  %D = select nsz nnan <vscale x 4 x i1> %cond, <vscale x 4 x float> %C, <vscale x 4 x float> %A
+  ret <vscale x 4 x float> %D
+}
+
+define <vscale x 4 x float> @select_nsz_fadd_nxv4f32_swapops(<vscale x 4 x i1> %cond, <vscale x 4 x float> %A, <vscale x 4 x float> %B) {
+; CHECK-LABEL: @select_nsz_fadd_nxv4f32_swapops(
+; CHECK-NEXT:    [[C:%.*]] = select fast <vscale x 4 x i1> [[COND:%.*]], <vscale x 4 x float> zeroinitializer, <vscale x 4 x float> [[B:%.*]]
+; CHECK-NEXT:    [[D:%.*]] = fadd fast <vscale x 4 x float> [[C]], [[A:%.*]]
+; CHECK-NEXT:    ret <vscale x 4 x float> [[D]]
+;
+  %C = fadd fast <vscale x 4 x float> %A, %B
+  %D = select fast <vscale x 4 x i1> %cond, <vscale x 4 x float> %A, <vscale x 4 x float> %C
+  ret <vscale x 4 x float> %D
+}
+
 define float @select_fmul(i1 %cond, float %A, float %B) {
 ; CHECK-LABEL: @select_fmul(
 ; CHECK-NEXT:    [[C:%.*]] = select i1 [[COND:%.*]], float [[B:%.*]], float 1.000000e+00
 ; CHECK-NEXT:    [[D:%.*]] = fmul float [[C]], [[A:%.*]]
 ; CHECK-NEXT:    ret float [[D]]
 ;
   %C = fmul float %A, %B
   %D = select i1 %cond, float %C, float %A

 ; CHECK-NEXT:    [[D:%.*]] = fsub fast float [[A:%.*]], [[C]]
 ; CHECK-NEXT:    ret float [[D]]
 ;
   %C = fsub fast float %A, %B
   %D = select i1 %cond, float %A, float %C
   ret float %D
 }
 
 define <4 x float> @select_nsz_fsub_v4f32(<4 x i1> %cond, <4 x float> %A, <4 x float> %B) {
 ; CHECK-LABEL: @select_nsz_fsub_v4f32(
-; CHECK-NEXT:    [[C:%.*]] = select <4 x i1> [[COND:%.*]], <4 x float> [[B:%.*]], <4 x float> zeroinitializer
+; CHECK-NEXT:    [[C:%.*]] = select nsz <4 x i1> [[COND:%.*]], <4 x float> [[B:%.*]], <4 x float> zeroinitializer
 ; CHECK-NEXT:    [[D:%.*]] = fsub <4 x float> [[A:%.*]], [[C]]
 ; CHECK-NEXT:    ret <4 x float> [[D]]
 ;
   %C = fsub <4 x float> %A, %B
   %D = select nsz <4 x i1> %cond, <4 x float> %C, <4 x float> %A
   ret <4 x float> %D
 }
 
 define <vscale x 4 x float> @select_nsz_fsub_nxv4f32(<vscale x 4 x i1> %cond, <vscale x 4 x float> %A, <vscale x 4 x float> %B) {
 ; CHECK-LABEL: @select_nsz_fsub_nxv4f32(
-; CHECK-NEXT:    [[C:%.*]] = select <vscale x 4 x i1> [[COND:%.*]], <vscale x 4 x float> [[B:%.*]], <vscale x 4 x float> zeroinitializer
+; CHECK-NEXT:    [[C:%.*]] = select nsz <vscale x 4 x i1> [[COND:%.*]], <vscale x 4 x float> [[B:%.*]], <vscale x 4 x float> zeroinitializer
 ; CHECK-NEXT:    [[D:%.*]] = fsub <vscale x 4 x float> [[A:%.*]], [[C]]
 ; CHECK-NEXT:    ret <vscale x 4 x float> [[D]]
 ;
   %C = fsub <vscale x 4 x float> %A, %B
   %D = select nsz <vscale x 4 x i1> %cond, <vscale x 4 x float> %C, <vscale x 4 x float> %A
   ret <vscale x 4 x float> %D
 }