[InstCombine] Optimize (+0.0 - A) + B

[4tXJ7f]

Summary

Before, InstCombiner::visitFAdd() was only optimizing (-0.0 - A) + B and
not (+0.0 - A) + B because InstCombiner::dyn_castFNegVal() by default
only matches negative floating-point zeroes even though +/- 0.0 should
behave the same in this situation. This patch changes the check to
accept positive zeros and fixes a discrepancy between the names of the
optional parameter in the header and the source file for
InstCombiner::dyn_castFNegVal().

[t.p.northover]

Is this a counter-example, or am I misreading?

#include <stdio.h>

int main() {
  float a = 0.0;
  float b = -0.0;
  printf("a = %f, b = %f\n", a, b);
  printf("b - a = %f\n", b - a);
  printf("(-0.0f - a) + b = %f\n", (-0.0f - a) + b);
  printf("(+0.0f - a) + b = %f\n", (+0.0f - a) + b);
}

[4tXJ7f]

Ouch, I think you are right. I tested it with a = +/- 0.0 but not with zeroes for both values, sorry about that. Do you think that it would be a good idea to add a testcase to document this behavior (check that (+0.0 - A) + B does not get optimized and have a comment to explain why)? Would it be a good idea to do the optimization for +0.0 if nsz is set? The optimizer currently optimizes:

%t0 = fsub nsz float +0.000000e+00, %x
%t1 = fadd float %t0, %y

(looks like +0.000000e+00 gets turned into -0.000000e+00 and then the other optimization applies) but not:

%t0 = fsub float +0.000000e+00, %x
t1 = fadd nsz float %t0, %y

Thanks a lot!

[t.p.northover]

Do you think that it would be a good idea to add a testcase to document this behavior (check that (+0.0 - A) + B does not get optimized and have a comment to explain why)?

Definitely. If you want to do that post-commit review would be fine. Commit away!

Would it be a good idea to do the optimization for +0.0 if nsz is set?

I think that would be fine, but I'm more of a dabbler in IEEE-754 than an expert. It's possible we have other canonicalizations that convert "fsub nsz 0.0, %x" into "fsub nsz -0.0, %x" (and then the usual case would take over). It's also possible that if we don't that would be a better approach anyway.

Cheers.

Tim.

lib/Transforms/InstCombine/InstCombineAddSub.cpp

if (isa<Constant>(RHS)) {
 
     if (SelectInst *SI = dyn_cast<SelectInst>(LHS))
       if (Instruction *NV = FoldOpIntoSelect(I, SI))
         return NV;
   }
 
   // -A + B  -->  B - A
   // -A + -B  -->  -(A + B)
-  if (Value *LHSV = dyn_castFNegVal(LHS)) {
+  if (Value *LHSV = dyn_castFNegVal(LHS, true)) {
     Instruction *RI = BinaryOperator::CreateFSub(RHS, LHSV);
     RI->copyFastMathFlags(&I);
     return RI;
   }
 
   // A + -B  -->  A - B
   if (!isa<Constant>(RHS))
     if (Value *V = dyn_castFNegVal(RHS)) {

lib/Transforms/InstCombine/InstCombineInternal.h

public:
   // Replace select with select operand SIOpd in SI-ICmp sequence when possible
   bool replacedSelectWithOperand(SelectInst *SI, const ICmpInst *Icmp,
                                  const unsigned SIOpd);
 
 private:
   bool ShouldChangeType(unsigned FromBitWidth, unsigned ToBitWidth) const;
   bool ShouldChangeType(Type *From, Type *To) const;
   Value *dyn_castNegVal(Value *V) const;
-  Value *dyn_castFNegVal(Value *V, bool NoSignedZero = false) const;
+  Value *dyn_castFNegVal(Value *V, bool IgnoreZeroSign = false) const;
   Type *FindElementAtOffset(PointerType *PtrTy, int64_t Offset,
                             SmallVectorImpl<Value *> &NewIndices);
   Instruction *FoldOpIntoSelect(Instruction &Op, SelectInst *SI);
 
   /// \brief Classify whether a cast is worth optimizing.
   ///
   /// Returns true if the cast from "V to Ty" actually results in any code
   /// being generated and is interesting to optimize out. If the cast can be

test/Transforms/InstCombine/fadd.ll

+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+; -A + B --> B - A
+define float @test1(float %x, float %y) {
+  %t0 = fsub float -0.000000e+00, %x
+  %t1 = fadd float %t0, %y
+  ret float %t1
+
+; CHECK-LABEL: @test1
+; CHECK-NEXT:  [[R:%[a-z0-9]*]] = fsub float %y, %x
+; CHECK-NEXT:  ret float  [[R]]
+}
+
+; -A + B --> B - A
+define float @test2(float %x, float %y) {
+  %t0 = fsub float +0.000000e+00, %x
+  %t1 = fadd float %t0, %y
+  ret float %t1
+
+; CHECK-LABEL: @test2
+; CHECK-NEXT:  [[R:%[a-z0-9]*]] = fsub float %y, %x
+; CHECK-NEXT:  ret float [[R]]
+}