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–1700

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–1700

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–1700

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

lib/Transforms/InstCombine/InstCombineCasts.cpp
1616–1617

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.