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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/IR/Constants.cpp:2853
       case Instruction::FAdd: // X + -0.0 = X
         // TODO: If the fadd has 'nsz', should we return +0.0?
+        return NSZ ? Constant::getNullValue(Ty)
----------------
Remove the TODO comment.


================
Comment at: llvm/lib/IR/Constants.cpp:2854-2855
         // TODO: If the fadd has 'nsz', should we return +0.0?
-        return ConstantFP::getNegativeZero(Ty);
+        return NSZ ? Constant::getNullValue(Ty)
+                   : ConstantFP::getNegativeZero(Ty);
       case Instruction::FMul: // X * 1.0 = X
----------------
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)


================
Comment at: llvm/test/Transforms/InstCombine/select-binop-foldable-floating-point.ll:66
+  %C = fadd <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
----------------
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?


================
Comment at: llvm/test/Transforms/InstCombine/select-binop-foldable-floating-point.ll:158
 
+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(
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126774/new/

https://reviews.llvm.org/D126774