This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Eliminate compares - add handling for logical operations without the use of condition registers
ClosedPublic

Authored by nemanjai on Apr 8 2017, 10:16 AM.

Details

Summary

This adds the code gen and testing for logical operations on i1 values without using condition registers (when possible).
It handles:

  • logical operations with SETCC inputs
  • logical operations with TRUNCATE inputs
  • logical operations with logical operations as inputs
  • zero extensions of logical operations on i1 values (as above)

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai created this revision.Apr 8 2017, 10:16 AM
nemanjai updated this revision to Diff 95086.Apr 13 2017, 1:34 AM
nemanjai retitled this revision from [PowerPC] Eliminate compares in instruction selection - Vol. 5 to [PowerPC] Eliminate compares - add handling for logical operations without the use of condition registers.
nemanjai edited the summary of this revision. (Show Details)
nemanjai added inline comments.Apr 19 2017, 8:45 AM
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2751 ↗(On Diff #95086)

These two variables were erroneously left in after I refactored the code to not need them. They'll be removed on the commit.

nemanjai updated this revision to Diff 98986.May 15 2017, 5:16 AM

This is a significantly scoped-down version of this patch to make it more manageable for review. It simply extends the use of the existing handling of ISD::SETCC in GPRs to their uses in logic operations (AND/OR/XOR).

Furthermore, this adds a restriction to the SETCC handling to only perform the expansion if all the uses need the value in a GPR since doing the expansion in such a situation would be a net addition of instructions without being able to eliminate the compare instruction.

echristo edited edge metadata.May 17 2017, 3:45 PM

A few inline comments, a couple of refactorings and some cleanup, otherwise looks OK.

One other comment: This is starting to feel like we're doing all of isel in C++ in the backend. I'd like to look at being able to do as much of this in generic code as possible. Perhaps take a look and see what we can manage that way and a comment at some point before all of this in the source?

Thanks!

lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2511 ↗(On Diff #98986)

Comment?

2569 ↗(On Diff #98986)

bool operand :)

2578 ↗(On Diff #98986)

The naming convention here is a little hard to parse later on. I'm seeing IsNot as !a and not a ^ -1...

2609 ↗(On Diff #98986)

Maybe call them LHS and RHS? :)

2662 ↗(On Diff #98986)

Needs the same commentary as the previous patch.

2814 ↗(On Diff #98986)

Description?

2850 ↗(On Diff #98986)

This should probably be an earlier return before you do the work above?

3207 ↗(On Diff #98986)

fallthrough?

nemanjai marked 8 inline comments as done.May 18 2017, 3:52 PM
nemanjai added inline comments.
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
3207 ↗(On Diff #98986)

Ah good catch. I missed that. Thanks.

nemanjai updated this revision to Diff 99505.May 18 2017, 3:56 PM
nemanjai marked an inline comment as done.

Addressed comments to add some documentation, remove a bool parameter and add the missing break in the switch.

echristo added inline comments.May 22 2017, 1:50 PM
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
280 ↗(On Diff #99505)

Part of asking for the enum is wondering why it's necessary. It seems odd to say "getLogicOpInGPR" to want to know whether or not to put something in a GPR.

nemanjai added inline comments.May 23 2017, 10:01 AM
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
280 ↗(On Diff #99505)

That's a good point. Perhaps the naming is a poor choice. I could rename it to performLogicOpInGPR().
The idea would be to convey the fact that this simply defers moving the data out of a GPR. What it does is take something like:
(and (i1 (setcc %a, %b, CC), (setcc %c, %d, CC2))) and convert it to a sequence that does the comparisons and the logical operation in a GPR.
That of course still leaves the question of whether we want the result of the logical operation itself to stay in a GPR (for further extended operations) or to be moved out to a CR (for use in branches, ISEL, etc.).

So I think maybe renaming the function and/or providing a clearer comment as to what's happening might be the right choice here. What do you think?

echristo added inline comments.May 23 2017, 11:46 AM
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
280 ↗(On Diff #99505)

I'd probably just separate out the performLogicOptInGPR and then just add code where you'd pass the bool argument to pull it into a CR if you need it after the call?

nemanjai updated this revision to Diff 100577.May 28 2017, 8:50 PM

This revision does away with the Boolean parameter to getLogicOpInGPR and adds the logic to extract the CR result at the only call of that function that needs it.
Furthermore, this patch eliminates bitwise negations if they're produced by instructions that have a record-form.

nemanjai marked an inline comment as done.May 28 2017, 8:51 PM

couple of inline comments.

lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2570 ↗(On Diff #100577)

Maybe "computeLogicOpInGPR"?

2701–2703 ↗(On Diff #100577)

I think you mean "single bit" rather than "CR bit" at least I'm reading it that way rather than in a cc register right?

nemanjai marked 2 inline comments as done.May 29 2017, 8:22 PM
nemanjai added inline comments.
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2570 ↗(On Diff #100577)

Will do.

2701–2703 ↗(On Diff #100577)

We have produced a record-form machine node above. The code immediately after this comment actually gets a single bit out of CR0 (which was set by the record-form op) and selects this logical operation to that bit. So it is a single bit, but it is in a CR - CR0 to be specific.
Perhaps it is clearer if I set the comment to this:

// Select this node to a single bit from CR0 set by the record-form node
// just created. For bitwise negation, use the EQ bit which is the equivalent
// of negating the result (i.e. it is a bit set when the result of the operation
// is zero).
nemanjai updated this revision to Diff 100657.May 29 2017, 8:32 PM
nemanjai marked 2 inline comments as done.

Address cosmetic comments - name change and line comment.

echristo accepted this revision.May 30 2017, 2:49 PM

SGTM. It still seems a bit overly complicated, but I don't think there's anything for it at the moment.

lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2701–2703 ↗(On Diff #100577)

SGTM.

This revision is now accepted and ready to land.May 30 2017, 2:49 PM
This revision was automatically updated to reflect the committed changes.