[PATCH] D143505: [InstSimplify] fix/improve folding with an SNaN operand

[Sanjay Patel via Phabricator via llvm-commits]

spatel created this revision.
spatel added reviewers: arsenm, kpn, jyknight, sepavloff, Muon.
Herald added subscribers: kosarev, StephenFan, hiraditya, tpr, mcrosier.
Herald added a project: All.
spatel requested review of this revision.
Herald added subscribers: llvm-commits, wdng.
Herald added a project: LLVM.

There are 2 issues here:

1. In the default LLVM FP environment (regular FP math instructions), SNaN is some flavor of "don't care" which we will nail down in D143074 <https://reviews.llvm.org/D143074>, so this is just a quality-of-implementation improvement for default FP.
2. In the constrained FP environment (constrained intrinsics), SNaN must not propagate through a math operation; it has to be quieted according to IEEE-754 spec. That is independent of exception handling mode, so the current behavior is a miscompile.

The "scalbn" hack to multiply by 1.0 to quiet the NaN is borrowed from code in the AMDGPU backend. I don't think there's a cleaner way to make that happen given the APFloat APIs, but suggestions welcome.


https://reviews.llvm.org/D143505

Files:
  llvm/lib/Analysis/InstructionSimplify.cpp
  llvm/test/Transforms/InstSimplify/fp-nan.ll
  llvm/test/Transforms/InstSimplify/strictfp-fadd.ll


Index: llvm/test/Transforms/InstSimplify/strictfp-fadd.ll
===================================================================
--- llvm/test/Transforms/InstSimplify/strictfp-fadd.ll
+++ llvm/test/Transforms/InstSimplify/strictfp-fadd.ll
@@ -370,17 +370,21 @@
   ret float %add
 }
 
+; Exceptions are ignored, so this can be folded, but constrained math requires that SNaN is quieted per IEEE-754 spec.
+
 define float @fold_fadd_snan_variable_ebignore(float %x) #0 {
 ; CHECK-LABEL: @fold_fadd_snan_variable_ebignore(
-; CHECK-NEXT:    ret float 0x7FF4000000000000
+; CHECK-NEXT:    ret float 0x7FFC000000000000
 ;
   %add = call float @llvm.experimental.constrained.fadd.f32(float 0x7ff4000000000000, float %x, metadata !"round.tonearest", metadata !"fpexcept.ignore") #0
   ret float %add
 }
 
+; Exceptions may (not) trap, so this can be folded, but constrained math requires that SNaN is quieted per IEEE-754 spec.
+
 define float @fold_fadd_snan_variable_ebmaytrap(float %x) #0 {
 ; CHECK-LABEL: @fold_fadd_snan_variable_ebmaytrap(
-; CHECK-NEXT:    ret float 0x7FF4000000000000
+; CHECK-NEXT:    ret float 0x7FFC000000000000
 ;
   %add = call float @llvm.experimental.constrained.fadd.f32(float 0x7ff4000000000000, float %x, metadata !"round.tonearest", metadata !"fpexcept.maytrap") #0
   ret float %add
Index: llvm/test/Transforms/InstSimplify/fp-nan.ll
===================================================================
--- llvm/test/Transforms/InstSimplify/fp-nan.ll
+++ llvm/test/Transforms/InstSimplify/fp-nan.ll
@@ -31,21 +31,21 @@
   ret float %r
 }
 
-; Signaling
+; Signaling - make quiet and preserve the payload and signbit
 
 define float @fsub_nan_op1(float %x) {
 ; CHECK-LABEL: @fsub_nan_op1(
-; CHECK-NEXT:    ret float 0x7FF1000000000000
+; CHECK-NEXT:    ret float 0x7FF9000000000000
 ;
   %r = fsub float %x, 0x7FF1000000000000
   ret float %r
 }
 
-; Signaling and signed
+; Signaling and signed - make quiet and preserve the payload and signbit
 
 define double @fmul_nan_op0(double %x) {
 ; CHECK-LABEL: @fmul_nan_op0(
-; CHECK-NEXT:    ret double 0xFFF0000000000001
+; CHECK-NEXT:    ret double 0xFFF8000000000001
 ;
   %r = fmul double 0xFFF0000000000001, %x
   ret double %r
Index: llvm/lib/Analysis/InstructionSimplify.cpp
===================================================================
--- llvm/lib/Analysis/InstructionSimplify.cpp
+++ llvm/lib/Analysis/InstructionSimplify.cpp
@@ -5309,13 +5309,15 @@
 /// Try to propagate existing NaN values when possible. If not, replace the
 /// constant or elements in the constant with a canonical NaN.
 static Constant *propagateNaN(Constant *In) {
-  if (auto *VecTy = dyn_cast<FixedVectorType>(In->getType())) {
+  Type *Ty = In->getType();
+  if (auto *VecTy = dyn_cast<FixedVectorType>(Ty)) {
     unsigned NumElts = VecTy->getNumElements();
     SmallVector<Constant *, 32> NewC(NumElts);
     for (unsigned i = 0; i != NumElts; ++i) {
       Constant *EltC = In->getAggregateElement(i);
       // Poison and existing NaN elements propagate.
       // Replace unknown or undef elements with canonical NaN.
+      // TODO: Quiet a signaling NaN element.
       if (EltC && (isa<PoisonValue>(EltC) || EltC->isNaN()))
         NewC[i] = EltC;
       else
@@ -5324,13 +5326,15 @@
     return ConstantVector::get(NewC);
   }
 
-  // It is not a fixed vector, but not a simple NaN either?
+  // If it is not a fixed vector, but not a simple NaN either, return a
+  // canonical NaN.
   if (!In->isNaN())
-    return ConstantFP::getNaN(In->getType());
+    return ConstantFP::getNaN(Ty);
 
-  // Propagate the existing NaN constant when possible.
-  // TODO: Should we quiet a signaling NaN?
-  return In;
+  // Propagate an existing QNaN constant. If it is an SNaN, make it quiet, but
+  // preserve the sign/payload.
+  const APFloat &NaN = cast<ConstantFP>(In)->getValue();
+  return ConstantFP::get(Ty, scalbn(NaN, 0, APFloat::rmNearestTiesToEven));
 }
 
 /// Perform folds that are common to any floating-point operation. This implies


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D143505.495552.patch
Type: text/x-patch
Size: 4017 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230207/65f09e9c/attachment.bin>