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

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally marked an inline comment as done.
cameron.mcinally 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);
----------------
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.


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

https://reviews.llvm.org/D67446