[PATCH] D67446: [ConstProp] allow folding for fma that produces NaN

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked an inline comment as done.
spatel added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:2242
+          V.fusedMultiplyAdd(Op2->getValueAPF(), Op3->getValueAPF(),
+                             APFloat::rmNearestTiesToEven);
+          return ConstantFP::get(Ty->getContext(), V);
----------------
cameron.mcinally wrote:
> spatel wrote:
> > cameron.mcinally wrote:
> > > spatel wrote:
> > > > reames wrote:
> > > > > 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.
> > > > 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?
> > > > 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.
> > 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.
> 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.
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
}

```


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

https://reviews.llvm.org/D67446