[InstSimplify] fix/improve folding with an SNaN operand

[spatel]

Summary

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, 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.

[arsenm]

llvm/lib/Analysis/InstructionSimplify.cpp

5337

Probably should add / expose a makeQuiet if this is going to spread more places

[spatel]

llvm/lib/Analysis/InstructionSimplify.cpp

5337

Yes, that was just laziness - will update.

[spatel]

Patch updated:
Added a "makeQuiet" wrapper to APFloat. If that part + AMDGPU diff looks ok, I can make it a pre-commit. It doesn't change any functionality for this patch.

[spatel]

Ping.

AFAIK, there is no opposition to the view that the current behavior is a miscompile for strict FP. This is independent of any documentation/behavioral changes for default FP that may result from D143074.

[jyknight]

llvm/lib/Analysis/InstructionSimplify.cpp

5320

Why is this a TODO? Is it more complex than just another call to makeQuiet?

[spatel]

llvm/lib/Analysis/InstructionSimplify.cpp

5320

I don't think so. It was just to have a minimal patch as a first step (the APFloat change wasn't in this patch in the first draft either).

I can add vector tests and make that a follow-up patch, or I can squash it all together if that's preferred.

[jyknight]

llvm/lib/Analysis/InstructionSimplify.cpp

5320

If you're sending a follow-up to fix this, too, then SGTM.

llvm/include/llvm/ADT/APFloat.h

public:
 
   /// A static helper to produce a copy of an APFloat value with its sign
   /// copied from some other APFloat.
   static APFloat copySign(APFloat Value, const APFloat &Sign) {
     Value.copySign(Sign);
     return Value;
   }
 
+  /// Assuming this is an IEEE-754 NaN value, quiet its signaling bit.
+  /// This preserves the sign and payload bits.
+  APFloat makeQuiet() const {
+    APFloat Result(*this);
+    Result.getIEEE().makeQuiet();
+    return Result;
+  }
+
   opStatus convert(const fltSemantics &ToSemantics, roundingMode RM,
                    bool *losesInfo);
   opStatus convertToInteger(MutableArrayRef<integerPart> Input,
                             unsigned int Width, bool IsSigned, roundingMode RM,
                             bool *IsExact) const {
     APFLOAT_DISPATCH_ON_SEMANTICS(
         convertToInteger(Input, Width, IsSigned, RM, IsExact));
   }

llvm/lib/Analysis/InstructionSimplify.cpp

 Value *llvm::simplifyFNegInst(Value *Op, FastMathFlags FMF,
                               const SimplifyQuery &Q) {
   return ::simplifyFNegInst(Op, FMF, Q, RecursionLimit);
 }
 
 /// 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
         NewC[i] = (ConstantFP::getNaN(VecTy->getElementType()));
     }
     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.
+  return ConstantFP::get(Ty, cast<ConstantFP>(In)->getValue().makeQuiet());
 }
 
 /// Perform folds that are common to any floating-point operation. This implies
 /// transforms based on poison/undef/NaN because the operation itself makes no
 /// difference to the result.
 static Constant *simplifyFPOp(ArrayRef<Value *> Ops, FastMathFlags FMF,
                               const SimplifyQuery &Q,
                               fp::ExceptionBehavior ExBehavior,
                               RoundingMode Rounding) {

llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp

if (C && (C->isZero() || C->isInfinity())) {
       return IC.replaceInstUsesWith(II, Op0);
     }
 
     // With strictfp, be more careful about possibly needing to flush denormals
     // or not, and snan behavior depends on ieee_mode.
     if (II.isStrictFP())
       break;
 
-    if (C && C->isNaN()) {
-      // FIXME: We just need to make the nan quiet here, but that's unavailable
-      // on APFloat, only IEEEfloat
-      auto *Quieted =
-          ConstantFP::get(Ty, scalbn(*C, 0, APFloat::rmNearestTiesToEven));
-      return IC.replaceInstUsesWith(II, Quieted);
-    }
+    if (C && C->isNaN())
+      return IC.replaceInstUsesWith(II, ConstantFP::get(Ty, C->makeQuiet()));
 
     // ldexp(x, 0) -> x
     // ldexp(x, undef) -> x
     if (isa<UndefValue>(Op1) || match(Op1, PatternMatch::m_ZeroInt())) {
       return IC.replaceInstUsesWith(II, Op0);
     }
 
     break;

llvm/test/Transforms/InstSimplify/fp-nan.ll

 define float @fsub_nan_op0(float %x) {
 ; CHECK-LABEL: @fsub_nan_op0(
 ; CHECK-NEXT:    ret float 0x7FFFFF0000000000
 ;
   %r = fsub float 0x7FFFFF0000000000, %x
   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
 }
 
 ; Vector type
 
 define <2 x float> @fmul_nan_op1(<2 x float> %x) {

llvm/test/Transforms/InstSimplify/strictfp-fadd.ll

 ; CHECK-LABEL: @fold_fadd_qnan_qnan_ebstrict(
 ; CHECK-NEXT:    [[ADD:%.*]] = call float @llvm.experimental.constrained.fadd.f32(float 0x7FF8000000000000, float 0x7FF8000000000000, metadata !"round.tonearest", metadata !"fpexcept.strict") #[[ATTR0]]
 ; CHECK-NEXT:    ret float 0x7FF8000000000000
 ;
   %add = call float @llvm.experimental.constrained.fadd.f32(float 0x7ff8000000000000, float 0x7ff8000000000000, metadata !"round.tonearest", metadata !"fpexcept.strict") #0
   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
 }
 
 define float @fold_fadd_snan_variable_ebstrict(float %x) #0 {
 ; CHECK-LABEL: @fold_fadd_snan_variable_ebstrict(
 ; CHECK-NEXT:    [[ADD:%.*]] = call float @llvm.experimental.constrained.fadd.f32(float 0x7FF4000000000000, float [[X:%.*]], metadata !"round.tonearest", metadata !"fpexcept.strict") #[[ATTR0]]