Page MenuHomePhabricator

[InstCombine] Use SimplifyFMulInst to simplify multiply in fma.
ClosedPublic

Authored by fhahn on Mon, Sep 9, 5:47 AM.

Details

Summary

This allows us to fold fma's that multiply with 0.0. Also, the
multiply by 1.0 case is handled there as well. The fneg/fabs cases
are not handled by SimplifyFMulInst, so we need to keep them.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Mon, Sep 9, 5:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Sep 9, 5:47 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
fhahn updated this revision to Diff 219329.Mon, Sep 9, 5:47 AM

Remove unrelated test-case.

Harbormaster completed remote builds in B37910: Diff 219329.
spatel added inline comments.Mon, Sep 9, 1:17 PM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2247–2252 ↗(On Diff #219329)

I don't understand what this block does. How does an fmul simplify to a different fmul? And then it becomes LHS^2?

2260–2262 ↗(On Diff #219329)

It would be easier to read if you move the simplify call below these local variables and then re-use those names.

llvm/test/Transforms/InstCombine/fma.ll
185 ↗(On Diff #219329)

I'm not seeing how these got commuted.

372–375 ↗(On Diff #219329)

Please add all of the new tests with baseline results as a preliminary commit, so we just have the diffs here.
Add one more test like:
call fast fma(sqrt(X), sqrt(X), Y) --> X*Y

fhahn updated this revision to Diff 219428.Mon, Sep 9, 1:44 PM

Address review comments, thanks!

fhahn marked 4 inline comments as done.Mon, Sep 9, 1:50 PM
fhahn added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2247–2252 ↗(On Diff #219329)

I don't understand what this block does. How does an fmul simplify to a different fmul?

If we simplify to a different fmul, we can update the existing FMA and we do not have to replace it with a new fmul + fadd. Not sure if that happens in practice though. Simplifications like the ones below ( -x * -y => x * y) could result in a different fmul. But currently we do not do this in SimplifyFMulInst. If there is a reason we never do create different fmuls there, then we can drop it.

And then it becomes LHS^2?

That should have been RHS of course.

2260–2262 ↗(On Diff #219329)

Sure, I've moved it down.

llvm/test/Transforms/InstCombine/fma.ll
185 ↗(On Diff #219329)

That was noise of update_test_checks.py I think. The way it generates checks for arguments, it will match both fmul %x, %y and fmul %y, %x.

I've stripped the unnecessary changes.

372–375 ↗(On Diff #219329)

Done

Looks good to me, but i'm not really good at llvm's FP.

spatel added inline comments.Mon, Sep 9, 3:10 PM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2247–2252 ↗(On Diff #219329)

I think we should drop this clause...unless there is a test that shows the intended transform?
InstSimplify never creates new instructions because it can be used as an analysis rather than transform pass.
If we fold to a different multiply, then it should be optimized just as well as.
Eg, we have this fold in InstCombiner::visitFMul():

// -X * -Y --> X * Y

(can't do that in instsimplify because it's a new instruction)

fhahn updated this revision to Diff 219509.Tue, Sep 10, 3:35 AM
fhahn edited the summary of this revision. (Show Details)

Remove fmul matching.

fhahn marked an inline comment as done.Tue, Sep 10, 3:36 AM
fhahn added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2247–2252 ↗(On Diff #219329)

Thanks for clarifying! I've dropped it.

spatel accepted this revision.Tue, Sep 10, 4:23 AM

LGTM

This revision is now accepted and ready to land.Tue, Sep 10, 4:23 AM
This revision was automatically updated to reflect the committed changes.