Page MenuHomePhabricator

[InstCombine] Update fptrunc (fneg x)) -> (fneg (fptrunc x) for unary FNeg
ClosedPublic

Authored by cameron.mcinally on May 29 2019, 2:18 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2019, 2:18 PM
craig.topper added inline comments.May 29 2019, 2:21 PM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
1625 ↗(On Diff #202054)

If you started with a unary fneg, shouldn't we end up with a unary fneg?

cameron.mcinally marked an inline comment as done.May 29 2019, 2:42 PM
cameron.mcinally added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
1625 ↗(On Diff #202054)

So this combine actually covers both unary and binary FNeg. m_FNeg(X) will match an FSub(-0.0,X). The end goal is that both will end up producing an unary FNeg.

But, yeah, I see what you're saying. I could dispatch on the operator type for now, but it would be code to clean up later. I suppose the BinaryOperator will have to be switched to an UnaryOperator in the future anyway, so I'm not sure how much work I'm saving.

Just thinking aloud, I don't have a preference...

cameron.mcinally marked an inline comment as not done.May 29 2019, 4:36 PM
cameron.mcinally added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
1625 ↗(On Diff #202054)

Thinking about this some more, you’re correct. It’s not safe to turn a unary FNeg into a binary FNeg.

I was thinking about this as a short term solution, but we should play it safe in case this does not get updated.

I’ll update the patch...

cameron.mcinally marked an inline comment as not done.

Fix the Unary/Binary operator issue.

Also note that this change will require UnaryOperator::CreateFNegFMF(...) from D62521, so cannot be committed until that is complete.

cameron.mcinally marked 3 inline comments as done.

Fix typo.

Forgot to update the tests.

Will hold off on making any other corrections until D62521 has landed...

Check hasOneUse() before performing combine.

Also note that this change will require UnaryOperator::CreateFNegFMF(...) from D62521, so cannot be committed until that is complete.

D62521 has landed, so this patch is ready for review...

Check hasOneUse() before performing combine.

That's an independent fix and needs a test?

Check hasOneUse() before performing combine.

That's an independent fix and needs a test?

It's actually covered by the new @unary_neg_trunc_op1_extra_use and existing @neg_trunc_op1_extra_use tests. I missed that each was producing an extra instruction in my Diff before the hasOneUse() change. I had failed to copy over that check to the new code from the old BinaryOperator code above this.

Also, I just noticed I failed to add context to this patch. Will do so now...

Add context to Diff.

cameron.mcinally marked an inline comment as done.Jun 10 2019, 10:05 AM
cameron.mcinally added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
1535 ↗(On Diff #203855)

This is the check that guarded the old BinaryOperator-only combine.

spatel accepted this revision.Jun 10 2019, 2:37 PM

LGTM

This revision is now accepted and ready to land.Jun 10 2019, 2:37 PM
This revision was automatically updated to reflect the committed changes.