This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Eliminate compares in instruction selection - Vol. 1
ClosedPublic

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

Details

Summary

This patch is the first in a series of 6 patches that split https://reviews.llvm.org/D31240 into more manageable chunks as well as include some additional work that depends on the patch posted by Lei.

The first 4 are really just a split of the functionality in that patch. None of the functionality is turned on in those patches, so they include no test cases. Patches 5 and 6 add further functionality and turn on what the original patch provided. So those two patches are the ones that include the test cases.

This particular one, just provides some of the infrastructure that the others build on - none of the functions here transform any compare instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai created this revision.Apr 8 2017, 10:00 AM
lei commandeered this revision.Apr 10 2017, 12:19 PM
lei edited reviewers, added: nemanjai; removed: lei.
spatel added a subscriber: spatel.Apr 10 2017, 12:45 PM
nemanjai commandeered this revision.Apr 12 2017, 3:24 AM
nemanjai edited reviewers, added: lei; removed: nemanjai.
nemanjai edited edge metadata.

Temporarily commandeering this just to update the patches as per @echristo's request.

nemanjai updated this revision to Diff 94948.Apr 12 2017, 3:28 AM

Updated to reflect some changes in how the big patch is split up as per Eric's request. This specific patch is NFC and can probably be quickly reviewed and committed. It essentially just adds all the methods that contain functionality as stubs. The subsequent patches will then add functionality for the methods and add the test cases. As such, they should be mostly independent of each other then.

lei commandeered this revision.Apr 17 2017, 12:58 PM
lei removed a reviewer: lei.
nemanjai commandeered this revision.May 2 2017, 1:50 PM
nemanjai edited reviewers, added: lei; removed: nemanjai.

Taking over this review so that we can update/review/commit it soon.

nemanjai updated this revision to Diff 97495.May 2 2017, 1:53 PM
  • Removed the static utility functions that weren't being used yet
  • Added codegen for one of the patterns
  • Added testing for that pattern
echristo edited edge metadata.May 2 2017, 1:54 PM

Let's delete any function that just returns false or SDValue() and see what that does to some of the code.

nemanjai updated this revision to Diff 97501.May 2 2017, 2:27 PM

Removed functions that do nothing.

echristo requested changes to this revision.May 4 2017, 10:10 PM

Hi Nemanja!

The patch looks really good, my inline comments notwithstanding. I think they're mostly cosmetic rather than anything else with a little bit of refactoring.

Thanks!

-eric

lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2497 ↗(On Diff #97501)

The PPC64 bit is because of testing or some other reason?

2522 ↗(On Diff #97501)

I'm typically not a fan of boolean arguments to functions (for a number of reasons). Can we refactor this to deal with not doing that? Either pass N down, or an enum with one of the 4 states?

2535–2541 ↗(On Diff #97501)

This could probably use some more detailed comments.

2539 ↗(On Diff #97501)

Feels like these could be separate and separately documented early returns based on the above booleans/variables?

2559–2560 ↗(On Diff #97501)

Feels like these could be separate and separately documented early returns based on the above booleans/variables?

2570–2571 ↗(On Diff #97501)

See my earlier comment on boolean variables. I'm pretty sure it'll help readability here as well.

2574 ↗(On Diff #97501)

I find it handy to describe the code you expect to return for these blocks.

2597 ↗(On Diff #97501)

Document each case with the return instructions that you're going to be emitting? (and other places)

2639–2640 ↗(On Diff #97501)

Ditto with the boolean arguments?

This revision now requires changes to proceed.May 4 2017, 10:10 PM
nemanjai updated this revision to Diff 98111.May 7 2017, 3:12 PM
nemanjai edited edge metadata.

Address review comments:

  • remove Boolean parameters
  • split and document early exits
  • document the code-gen for various setcc condition codes in each case
  • document the reason this isn't supported on 32-bit targets

Few more inline comments :)

lib/Target/PowerPC/PPCISelDAGToDAG.cpp
264–265 ↗(On Diff #98111)

These need documenting as to what each of them means.

2496–2498 ↗(On Diff #98111)

This sounds more like "This code is currently specialized to 64-bit registers" rather than "this transformation doesn't make sense on 32-bit" yes?

2511 ↗(On Diff #98111)

No need for an else here. Can just leave it as an early return.

2512 ↗(On Diff #98111)

This can probably just be an if statement or a ternary operator in the call and get rid of ConvOpts as a local.

2516–2520 ↗(On Diff #98111)

Do we expect to do this with non-32bit quantities? It also seems that we don't do anything for NoOp and so can early return? (Reading down through addExtOrTrunc). Are you actually getting things like zext/sext 32->32?

2521 ↗(On Diff #98111)

This can be a few lines up.

2524 ↗(On Diff #98111)

Can probably hoist the N->getOptcode() == ISD::SIGN_EXTEND up to the beginning of the function.

2561 ↗(On Diff #98111)

May wish to assert that the input is MVT:i32?

2568–2569 ↗(On Diff #98111)

Let's go ahead and sink these two down to where they're used.

2586 ↗(On Diff #98111)

Did we want an i32 assertion in this function as well?

2604–2610 ↗(On Diff #98111)

Probably want to do something like:

if (cond) {

<stuff>
return SDValue(...)

}

assert(otherCond);
<stuff>
return SDValue(...);

2683 ↗(On Diff #98111)

4 and 2?

2684 ↗(On Diff #98111)

MVT InputVT

2685 ↗(On Diff #98111)

Do you mean '||' maybe?

2690 ↗(On Diff #98111)

Early returns if not 32bit inputs? It'll probably clean up the logic below as well. Also a bit confusing with the InputVT stuff above.

4810 ↗(On Diff #98111)

Cut and paste? :)

nemanjai added inline comments.May 9 2017, 6:01 PM
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2497 ↗(On Diff #97501)

We really only want to do this on 64-bit targets. The reason is that instructions sequences in many cases need to be different in 32-bit mode. This is a result of:

  1. Registers not actually being 32 bits wide in 32-bit mode
  2. The i64 values not fitting in single registers in 32-bit mode

I will add a comment to these early exits to this end.

264–265 ↗(On Diff #98111)

OK. I'll document them.

2496–2498 ↗(On Diff #98111)

Yes, the transformation as implemented is specialized for 64-bit targets. I'll update the comment.

2511 ↗(On Diff #98111)

Makes sense. Not sure why I left it in.

2512 ↗(On Diff #98111)

OK, I'll get rid of the temp.

2516–2520 ↗(On Diff #98111)

The complete patch will cover all cases. Right now we really only do 32-bit comparisons and we may or may not want the value in a 64-bit register (depending on the result type). But eventually we'll have all the combinations:
compare 32-bit values -> produce 32-bit value
compare 32-bit values -> produce 64-bit value
compare 64-bit values -> produce 32-bit value
compare 64-bit values -> produce 64-bit value

Two of those don't require conversions, one requires an extension and the other a truncation.

2521 ↗(On Diff #98111)

OK, makes sense.

2524 ↗(On Diff #98111)

We don't want to increment the stats in the early exit cases.

2561 ↗(On Diff #98111)

Will do. But at the beginning of the function. I'm not sure why the comment is at the end of the function.

2586 ↗(On Diff #98111)

Yup.

2604–2610 ↗(On Diff #98111)

Yeah. When I refactored, I missed this weird setup in this function.

2683 ↗(On Diff #98111)

Yup. 4 and 2. Just the operand numbers for the condition code in the two nodes. I'll rewrite this to name the operand number based on which node it is. Something like:
int CCOpNum = Compare.getOpcode() == ISD::SELECT_CC ? 4 : 2;

2684 ↗(On Diff #98111)

I thought we typically used the EVT class for this type of thing but sure.

2685 ↗(On Diff #98111)

Nope. Well I suppose I could DeMorgan-ize it to something like:

if (!(InputVT == MVT::i32 || InputVT == MVT::i64))
but I don't find that any more readable.

Ultimately, I want to exit early if it isn't one of those two types.

2690 ↗(On Diff #98111)

I suppose I can clean that up on this patch since it currently doesn't handle 64-bit inputs, but it'll just be right back as soon as I add it in one of the subsequent patches.

4810 ↗(On Diff #98111)

That's bizarre. No idea how I did that. I'll of course remove it :).

nemanjai added inline comments.May 9 2017, 6:35 PM
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2684 ↗(On Diff #98111)

On second thought, looks like it'll have to stay an EVT or else I'd have to call getSimpleType() on the result of getValueType().

nemanjai updated this revision to Diff 98384.May 9 2017, 6:38 PM

Addressed more inline comments.
Fixed up some early exits, added documentation, etc.

echristo accepted this revision.May 10 2017, 11:21 AM

Couple of small comment requests, but that's it.

Thanks!

-eric

lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2496–2498 ↗(On Diff #98111)

"We only do this for 64-bit targets for now." maybe?

2516–2520 ↗(On Diff #98111)

Interesting. I'll just take this on faith :)

2683 ↗(On Diff #98111)

Oh. Sure. That makes a lot more sense. Can you add a comment as well?

2684 ↗(On Diff #98111)

*shrug* Sure :)

2685 ↗(On Diff #98111)

k.

This revision is now accepted and ready to land.May 10 2017, 11:21 AM
This revision was automatically updated to reflect the committed changes.