This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Eliminate compares - add i32 sext/zext handling for SETLE/SETGE
ClosedPublic

Authored by nemanjai on Jun 9 2017, 12:15 AM.

Details

Summary

This patch adds a bit more of the infrastructure for doing comparisons in GPR's. It provides GE/LE patterns for i32's as well as the computation for GE/LE comparisons against zero for both i32 and i64.

Posting this for review since it's not just a patch that adds patterns - there's a bit of additional infrastructure.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai created this revision.Jun 9 2017, 12:15 AM
nemanjai added inline comments.
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2868 ↗(On Diff #101996)

OK, I realize this shouldn't have been handled "above above". The commit will have just one "above". :)

nemanjai added inline comments.Jun 13 2017, 5:17 AM
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2816 ↗(On Diff #101996)

I realize that the three by-ref bool's makes this signature look kind of busy. Perhaps it would be better to just encode these as bits in a single char or something along those lines?

echristo added inline comments.Jun 15 2017, 6:58 PM
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2816 ↗(On Diff #101996)

You don't appear to use the values anywhere after they're set or am I missing something?

nemanjai marked an inline comment as done.Aug 2 2017, 3:20 AM
nemanjai added inline comments.
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2816 ↗(On Diff #101996)

The values are used after LLVM_FALLTHROUGH. For example line 2937, etc.

nemanjai updated this revision to Diff 109313.Aug 2 2017, 3:27 AM

Rebased and fixed the assert message.

nemanjai added inline comments.Aug 2 2017, 3:31 AM
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2890 ↗(On Diff #109313)

IsRHSOne/IsRHSNegOne are not currently used but will be used to produce the LHS <= 0 sequence for LHS < 1 and the LHS >= 0 sequence for LHS > -1 in a subsequent patch.

Rather than omitting them from this patch and the function that re-sets them, I left them in. This way if this isn't how we want to implement swapAndReset(), I can address it in this patch rather than having to re-work it in the next.

echristo added inline comments.Aug 2 2017, 1:41 PM
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2890 ↗(On Diff #109313)

I don't like committing dead code so let's try to avoid that.

The swapAndReset function is a little unusual. I'd probably just suggest grabbing the values you need as you need them rather than breaking it out into a function. Depending on how it looks I'd just comment the swap and fall through.

Thoughts?

nemanjai added inline comments.Aug 2 2017, 2:06 PM
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2890 ↗(On Diff #109313)

Actually, I think that makes sense. At every place where we'd call that function, we just need to swap and set one of the three Boolean variables. I'll get rid of the function and put the code in the caller.

nemanjai updated this revision to Diff 109543.Aug 3 2017, 6:46 AM

Remove the odd swapAndReset() function.

echristo accepted this revision.Aug 3 2017, 1:29 PM

One inline comment, otherwise lgtm.

-eric

lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2913 ↗(On Diff #109543)

Comment on these that we're going to swap and fallthrough?

This revision is now accepted and ready to land.Aug 3 2017, 1:29 PM
This revision was automatically updated to reflect the committed changes.