Page MenuHomePhabricator

[FPEnv] Last BinaryOperator::isFNeg(...) to m_FNeg(...) changes
ClosedPublic

Authored by cameron.mcinally on Oct 24 2018, 8:42 AM.

Details

Summary

Continuing work from D52934 and D53205...

These are the last two BinaryOperator::isFNeg(...) references. Replace them with m_FNeg(...) to avoid regressions when we separate FNeg from the FSub IR instruction.

Diff Detail

Repository
rL LLVM

Event Timeline

Nice - that wasn't as bad as I was fearing. :)
I'm not sure how to expose a test diff with the fast-isel change (some combine appears to already be undef aware), but here's an instcombine test:
rL345206

lib/CodeGen/SelectionDAG/FastISel.cpp
1697 ↗(On Diff #170906)

Use match() here, so we can get rid of getFNegArgument() too?

Updating diff based on Sanjay's suggestions. One comment inline...

cameron.mcinally marked an inline comment as done.Oct 25 2018, 8:08 AM
cameron.mcinally added inline comments.
lib/CodeGen/SelectionDAG/FastISel.cpp
1697 ↗(On Diff #170906)

This would have been a weird use of match(...), i.e. ignoring the return value, so I refactored the code to pull the match(...) into selectFNeg(...). This is also a little weird, but seems ok. Thoughts or alternatives?

spatel added inline comments.Oct 25 2018, 8:14 AM
lib/CodeGen/SelectionDAG/FastISel.cpp
1697 ↗(On Diff #170906)

This seems fine to me (definitely better than having 2 match() calls).

lib/Transforms/InstCombine/InstCombineCasts.cpp
1616–1617 ↗(On Diff #171100)

Did the test that I added fail with this diff?

cameron.mcinally marked an inline comment as done.

Whoops, forgot to update. Yeah, your vector test looks good now.

spatel accepted this revision.Oct 25 2018, 10:18 AM

LGTM

This revision is now accepted and ready to land.Oct 25 2018, 10:18 AM
This revision was automatically updated to reflect the committed changes.