Page MenuHomePhabricator

[ConstProp] allow folding for fma that produces NaN
ClosedPublic

Authored by spatel on Wed, Sep 11, 7:30 AM.

Details

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(+∞, −∞)

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Wed, Sep 11, 7:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Sep 11, 7:30 AM
arsenm accepted this revision.Wed, Sep 11, 7:49 AM

LGTM

This revision is now accepted and ready to land.Wed, Sep 11, 7:49 AM
cameron.mcinally added inline comments.
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 accepted this revision.Wed, Sep 11, 8:06 AM
cameron.mcinally added inline comments.
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 added inline comments.Wed, Sep 11, 9:10 AM
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 marked an inline comment as done.Wed, Sep 11, 9:52 AM
spatel added inline comments.
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 marked an inline comment as done.Wed, Sep 11, 10:01 AM
cameron.mcinally added inline comments.
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 marked an inline comment as done.Wed, Sep 11, 10:05 AM
spatel added inline comments.
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 updated this revision to Diff 219740.Wed, Sep 11, 10:17 AM

Patch updated:
Added clarifying comment to APFloat header.

cameron.mcinally marked an inline comment as done.Wed, Sep 11, 11:09 AM
cameron.mcinally added inline comments.
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 marked an inline comment as done.Thu, Sep 12, 6:09 AM
spatel added inline comments.
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 marked an inline comment as done.Thu, Sep 12, 7:09 AM
cameron.mcinally added inline comments.
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...

This revision was automatically updated to reflect the committed changes.