[ConstProp] allow folding for fma that produces NaN

[spatel]

Summary

Folding for fma/fmuladd was added here:
rL202914
...and as seen in existing/unchanged tests, that works to propagate NaN if it's already an input, but we should fold an fma() that creates NaN too.

From IEEE-754-2008 7.2 "Invalid Operation", there are 2 clauses that apply to fma, so I added tests for those patterns:

c) fusedMultiplyAdd: fusedMultiplyAdd(0, ∞, c) or fusedMultiplyAdd(∞, 0, c) unless c is a quiet NaN; if c is a quiet NaN then it is implementation defined whether the invalid operation exception is signaled
d) addition or subtraction or fusedMultiplyAdd: magnitude subtraction of infinities, such as: addition(+∞, −∞)

[arsenm]

LGTM

[cameron.mcinally]

llvm/test/Transforms/ConstProp/fma.ll

196 ↗

(On Diff #219711)

Should this class of FMA return poison? There's no valid result for fma(0, inf, C), where C != NaN.

@eli.friedman

[cameron.mcinally]

llvm/test/Transforms/ConstProp/fma.ll

196 ↗

(On Diff #219711)

Bah, never mind. That only applies to converts that return integer results. FP results are defined as NaNs.

For operations producing results in floating-point format, the default result of an operation that signals the invalid operation exception shall be a quiet NaN that should provide some diagnostic information (see 6.2).

[reames]

llvm/lib/Analysis/ConstantFolding.cpp

2242 ↗

(On Diff #219711)

Does opInvalidOp always imply Nan? If so, then the name should be updated or at least clarifying comments added to the APFloat header. If not, then this code may be incorrect.

[spatel]

llvm/lib/Analysis/ConstantFolding.cpp

2242 ↗

(On Diff #219711)

From the perspective/usage of fusedMultiplyAdd() (and I think for any of the calls that produces an FP result), APFloat::opInvalidOp implies producing a NaN.

In the general case, APFloat::opInvalidOp does not necessarily imply NaN because we use that status for non-FP APIs. For example, we have this for IEEEFloat::convertToInteger():
"we provide deterministic values in case of an invalid operation exception, namely zero for NaNs and the minimal or maximal value respectively for underflow or overflow."

So I think this code is correct. Add something like the above text to the header comment?

[cameron.mcinally]

llvm/lib/Analysis/ConstantFolding.cpp

2242 ↗

(On Diff #219711)

In the general case, APFloat::opInvalidOp does not necessarily imply NaN because we use that status for non-FP APIs. For example, we have this for IEEEFloat::convertToInteger():
"we provide deterministic values in case of an invalid operation exception, namely zero for NaNs and the minimal or maximal value respectively for underflow or overflow."

There's been some discussion about returning poison for these cases wrt the constrained intrinsics. I don't know if a final decision was made. Just FYI.

[spatel]

llvm/lib/Analysis/ConstantFolding.cpp

2242 ↗

(On Diff #219711)

Also note from IEEE-754:
"For operations producing results in floating-point format, the default result of an operation that signals the invalid operation exception shall be a quiet NaN."
...since APFloat models that spec, if fusedMultiplyAdd() or any other FP math raises invalidOp status, but then does *not* produce a NaN, I'd say that's a bug in APFloat.

[spatel]

Patch updated:
Added clarifying comment to APFloat header.

[cameron.mcinally]

llvm/lib/Analysis/ConstantFolding.cpp

2242 ↗

(On Diff #219711)

Digressing a bit, but APFloat does not handle some SNaNs operands appropriately either. A SNaN operand to a signaling operation should return an InvalidOp+QNaN. If I'm not mistaken, I've seen a few cases where SNaN operands return APFloat::opOk.

[spatel]

llvm/lib/Analysis/ConstantFolding.cpp

2242 ↗

(On Diff #219711)

Good to know; my guess is that NaN propagation hasn't been looked at closely given the non-strict approach of general LLVM IR.

I think this FMA case is working as expected though:

define double @inf_times_zero_plus_signalling_nan()  {
; CHECK-LABEL: @inf_times_zero_plus_signalling_nan(
; CHECK-NEXT:    ret double 0x7FF8000000000000
;
  %1 = call double @llvm.fma.f64(double 0x7FF0000000000000, double -0.0, double 0x7FF0000000000001)
  ret double %1
}

[cameron.mcinally]

llvm/lib/Analysis/ConstantFolding.cpp

2242 ↗

(On Diff #219711)

IIRC, the result is correct, but APFloat returns an opOk. It should return an opInvalid. I can whip up a test case, if you'd like.

In any event, the SNaN issue is a general problem. It shouldn't hold this patch up...

llvm/trunk/include/llvm/ADT/APFloat.h

enum roundingMode {
     rmTowardNegative,
     rmTowardZero,
     rmNearestTiesToAway
   };
 
   /// IEEE-754R 7: Default exception handling.
   ///
   /// opUnderflow or opOverflow are always returned or-ed with opInexact.
+  ///
+  /// APFloat models this behavior specified by IEEE-754:
+  ///   "For operations producing results in floating-point format, the default
+  ///    result of an operation that signals the invalid operation exception
+  ///    shall be a quiet NaN."
   enum opStatus {
     opOK = 0x00,
     opInvalidOp = 0x01,
     opDivByZero = 0x02,
     opOverflow = 0x04,
     opUnderflow = 0x08,
     opInexact = 0x10
   };

llvm/trunk/lib/Analysis/ConstantFolding.cpp

static Constant *ConstantFoldScalarCall3(StringRef Name,
   if (const auto *Op1 = dyn_cast<ConstantFP>(Operands[0])) {
     if (const auto *Op2 = dyn_cast<ConstantFP>(Operands[1])) {
       if (const auto *Op3 = dyn_cast<ConstantFP>(Operands[2])) {
         switch (IntrinsicID) {
         default: break;
         case Intrinsic::fma:
         case Intrinsic::fmuladd: {
           APFloat V = Op1->getValueAPF();
-          APFloat::opStatus s = V.fusedMultiplyAdd(Op2->getValueAPF(),
-                                                   Op3->getValueAPF(),
-                                                   APFloat::rmNearestTiesToEven);
-          if (s != APFloat::opInvalidOp)
-            return ConstantFP::get(Ty->getContext(), V);
-
-          return nullptr;
+          V.fusedMultiplyAdd(Op2->getValueAPF(), Op3->getValueAPF(),
         }
         }
       }
     }
   }
 
   if (const auto *Op1 = dyn_cast<ConstantInt>(Operands[0])) {
     if (const auto *Op2 = dyn_cast<ConstantInt>(Operands[1])) {

llvm/trunk/test/Transforms/ConstProp/fma.ll

;
   %1 = call double @llvm.fma.f64(double 7.0, double 0xFFF0000000000000, double 0.0)
   ret double %1
 }
 
 ; -inf + inf --> NaN
 
 define double @inf_product_opposite_inf_addend_1()  {
 ; CHECK-LABEL: @inf_product_opposite_inf_addend_1(
-; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.fma.f64(double 7.000000e+00, double 0xFFF0000000000000, double 0x7FF0000000000000)
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    ret double 0x7FF8000000000000
 ;
   %1 = call double @llvm.fma.f64(double 7.0, double 0xFFF0000000000000, double 0x7FF0000000000000)
   ret double %1
 }
 
 ; inf + -inf --> NaN
 
 define double @inf_product_opposite_inf_addend_2()  {
 ; CHECK-LABEL: @inf_product_opposite_inf_addend_2(
-; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.fma.f64(double 7.000000e+00, double 0x7FF0000000000000, double 0xFFF0000000000000)
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    ret double 0x7FF8000000000000
 ;
   %1 = call double @llvm.fma.f64(double 7.0, double 0x7FF0000000000000, double 0xFFF0000000000000)
   ret double %1
 }
 
 ; -inf + inf --> NaN
 
 define double @inf_product_opposite_inf_addend_3()  {
 ; CHECK-LABEL: @inf_product_opposite_inf_addend_3(
-; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.fma.f64(double 0xFFF0000000000000, double 4.200000e+01, double 0x7FF0000000000000)
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    ret double 0x7FF8000000000000
 ;
   %1 = call double @llvm.fma.f64(double 0xFFF0000000000000, double 42.0, double 0x7FF0000000000000)
   ret double %1
 }
 
 ; inf + -inf --> NaN
 
 define double @inf_product_opposite_inf_addend_4()  {
 ; CHECK-LABEL: @inf_product_opposite_inf_addend_4(
-; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.fma.f64(double 0x7FF0000000000000, double 4.200000e+01, double 0xFFF0000000000000)
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    ret double 0x7FF8000000000000
 ;
   %1 = call double @llvm.fma.f64(double 0x7FF0000000000000, double 42.0, double 0xFFF0000000000000)
   ret double %1
 }
 
 ; 0 * -inf --> NaN
 
 define double @inf_times_zero_1()  {
 ; CHECK-LABEL: @inf_times_zero_1(
-; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.fma.f64(double 0.000000e+00, double 0xFFF0000000000000, double 4.200000e+01)
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    ret double 0x7FF8000000000000
 ;
   %1 = call double @llvm.fma.f64(double 0.0, double 0xFFF0000000000000, double 42.0)
   ret double %1
 }
 
 ; 0 * inf --> NaN
 
 define double @inf_times_zero_2()  {
 ; CHECK-LABEL: @inf_times_zero_2(
-; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.fma.f64(double 0.000000e+00, double 0x7FF0000000000000, double 4.200000e+01)
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    ret double 0x7FF8000000000000
 ;
   %1 = call double @llvm.fma.f64(double 0.0, double 0x7FF0000000000000, double 42.0)
   ret double %1
 }
 
 ; -inf * 0 --> NaN
 
 define double @inf_times_zero_3()  {
 ; CHECK-LABEL: @inf_times_zero_3(
-; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.fma.f64(double 0xFFF0000000000000, double 0.000000e+00, double 4.200000e+01)
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    ret double 0x7FF8000000000000
 ;
   %1 = call double @llvm.fma.f64(double 0xFFF0000000000000, double 0.0, double 42.0)
   ret double %1
 }
 
 ; inf * 0 --> NaN
 
 define double @inf_times_zero_4()  {
 ; CHECK-LABEL: @inf_times_zero_4(
-; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.fma.f64(double 0x7FF0000000000000, double 0.000000e+00, double 4.200000e+01)
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    ret double 0x7FF8000000000000
 ;
   %1 = call double @llvm.fma.f64(double 0x7FF0000000000000, double 0.0, double 42.0)
   ret double %1
 }
 
 ; -0 * -inf --> NaN
 
 define double @inf_times_zero_5()  {
 ; CHECK-LABEL: @inf_times_zero_5(
-; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.fma.f64(double -0.000000e+00, double 0xFFF0000000000000, double 4.200000e+01)
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    ret double 0x7FF8000000000000
 ;
   %1 = call double @llvm.fma.f64(double -0.0, double 0xFFF0000000000000, double 42.0)
   ret double %1
 }
 
 ; -0 * inf --> NaN
 
 define double @inf_times_zero_6()  {
 ; CHECK-LABEL: @inf_times_zero_6(
-; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.fma.f64(double -0.000000e+00, double 0x7FF0000000000000, double 4.200000e+01)
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    ret double 0x7FF8000000000000
 ;
   %1 = call double @llvm.fma.f64(double -0.0, double 0x7FF0000000000000, double 42.0)
   ret double %1
 }
 
 ; -inf * -0 --> NaN
 
 define double @inf_times_zero_7()  {
 ; CHECK-LABEL: @inf_times_zero_7(
-; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.fma.f64(double 0xFFF0000000000000, double -0.000000e+00, double 4.200000e+01)
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    ret double 0x7FF8000000000000
 ;
   %1 = call double @llvm.fma.f64(double 0xFFF0000000000000, double -0.0, double 42.0)
   ret double %1
 }
 
 ; inf * -0 --> NaN
 
 define double @inf_times_zero_8()  {
 ; CHECK-LABEL: @inf_times_zero_8(
-; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.fma.f64(double 0x7FF0000000000000, double -0.000000e+00, double 4.200000e+01)
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    ret double 0x7FF8000000000000
 ;
   %1 = call double @llvm.fma.f64(double 0x7FF0000000000000, double -0.0, double 42.0)
   ret double %1
 }
-