[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:
> > 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.


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

https://reviews.llvm.org/D67446