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


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

https://reviews.llvm.org/D67446