Page MenuHomePhabricator

[InstCombine] Simplify fma multiplication to nan for undef or nan operands.
ClosedPublic

Authored by fhahn on Oct 1 2019, 3:34 AM.

Details

Summary

In similar fashion to D67721, we can simplify FMA multiplications if any
of the operands is NaN or undef. In instcombine, we will simplify the
FMA to an fadd with a NaN operand, which in turn gets folded to NaN.

Note that this just changes SimplifyFMAFMul, so we still not catch the
case where only the Add part of the FMA is Nan/Undef.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Oct 1 2019, 3:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2019, 3:34 AM
cameron.mcinally accepted this revision.Oct 1 2019, 8:55 AM

LGTM

llvm/lib/Analysis/InstructionSimplify.cpp
4612 ↗(On Diff #222578)

Side note: these 2 transforms are not correct for X=Inf. I believe that they should return a QNaN in that case.

This revision is now accepted and ready to land.Oct 1 2019, 8:55 AM
fhahn marked an inline comment as done.Oct 1 2019, 12:42 PM
fhahn added inline comments.
llvm/lib/Analysis/InstructionSimplify.cpp
4612 ↗(On Diff #222578)

Yep thanks, I think we are missing FMF.noInfs, right?

cameron.mcinally marked an inline comment as done.Oct 1 2019, 2:00 PM
cameron.mcinally added inline comments.
llvm/lib/Analysis/InstructionSimplify.cpp
4612 ↗(On Diff #222578)

I think that's right.

I'm a little swamped on other projects right now, but I can add it to my TODO list if you'd like. Your call.

spatel added inline comments.Oct 1 2019, 2:31 PM
llvm/lib/Analysis/InstructionSimplify.cpp
4612 ↗(On Diff #222578)

The reasoning here and in at least 1 other fold in instsimplify that I know of is relying on the definition provided in the LangRef:
"Allow optimizations to assume the arguments and result are not NaN"

0 * Inf produces NaN, but that's not an allowable result because of the 'nnan' on this fmul. Therefore, an infinity input can be implicitly disregarded.

llvm/lib/Analysis/InstructionSimplify.cpp
4612 ↗(On Diff #222578)

Oh, that does make sense. Thanks for the clarification. And sorry for the noise.

This revision was automatically updated to reflect the committed changes.