Page MenuHomePhabricator

[GISel][RFC]: GlobalISel Combiner prototype
ClosedPublic

Authored by aditya_nandakumar on Dec 18 2017, 5:34 PM.

Details

Summary

Hello All

Here's a prototype for GISel Combiner pass/framework.
The various components are

  1. GICombinerHelper - this contains transformations that are common to all targets. Targets can pick and choose which transformations (at function/opcode granularity) each pass uses via configuring a GICombinerInfo.
  2. GICombiner - this contains some common code and it does the traversal, driving of combines, worklist management and iterating until convergence.
  3. GICombinerInfo - the GICombinerInfo is an interface with a virtual method called combine. The combiner info will allow targets to pick and choose (or implement their own specific combines). CombineInfos can make use of available combines in GICombineHelper to configure the transformations for a particular pass. Currently this approach allows cherry picking transformations from helpers (at function/opcode granularity) and also allows early returning on specific transformations. Targets also get to prioritize whether target specific combines run before/after the opt-in generic combines. Ideally we would like this part to be configured by both C++ and Tablegen. The CombinerInfo also has a field which indicates how to deal with IllegalOps (ie - should we allow to create them/or legalize them?). This has not been addressed in this patch as I'm not sure what the right approach here is.
  4. A CombinerPass would configure a CombinerInfo, create the GICombiner with the Info, and call GICombiner::combineMachineInstrs(MachineFunction&).

The above organization is very similar to the GISelLegalizer.

This patch includes a CombinerPass called PreLegalizeCombiner which is meant to run before legalization. Currently the only implemented transformation in the helper is simple copy propagation.

This patch also includes an initial version of InstCombine like PatternMatcher for easy matching of combines along with unit tests.

Looking forward to your feedback.

Diff Detail

Event Timeline

Unrelated AArch64 change snuck in. Removed..

Hi Aditya,

The overall design looks good to me. Obviously given that's a prototype we still miss a bunch of comment, but I like the general approach.
I've put a few nitpicks here and there, but don't pay to much attention to them, I expect you will polish the patch when more people are on board.

Two things I'd like to call out:

  1. When making this a real thing, we would need a patch in the GISel doc to explain the design and how it is intended to use
  2. I am guessing the PreLegalizerPass is just to show case the prototype, other I think it misses two things: A. The relationship with GICombiner is strange to me and should be reworked IMO (see the comment on the PreLegalizeCombiner class itself) B. The target should have a way to inject/choose its own combines for the pass (I actually believe that if a target needs to do that it would be better of creating its own prelegalize pass)

Anyhow, thanks for putting that up. This looks promising!

Cheers,
-Quentin

include/llvm/CodeGen/GlobalISel/GICombiner.h
1 ↗(On Diff #127443)

Unless it conflicts with something else, I would drop GI in the name. It already lives in the GlobalISel directory.

12 ↗(On Diff #127443)

Use doxygen style comments

24 ↗(On Diff #127443)

All the includes but MachineIRBuilder could be removed.
Use forward declarations for the type definitions.

31 ↗(On Diff #127443)

Why is the combiner info not const here, ditto for TPC?

Basically, I am guessing there are good reasons (I didn't look at the implementation yet), but the comments should make it clear from the API.

Also, is it valid to give nullptr for TPC?
If not, we should use a reference as well.

include/llvm/CodeGen/GlobalISel/GICombinerHelper.h
1 ↗(On Diff #127443)

Ditto for the GI prefix.

29 ↗(On Diff #127443)

The builder comes with MRI already (getMF().getRegInfo()).
Should we still have it being an additional argument.

34 ↗(On Diff #127443)

Is MI supposed to be a COPY or does this combine will do the check?

Basically, what is the precondition here.

36 ↗(On Diff #127443)

We would need to document the API. I.e., what does the boolean mean? In particular, what happens to MI is a change has been made?

include/llvm/CodeGen/GlobalISel/GICombinerInfo.h
28 ↗(On Diff #127443)

I'm guessing we would need an assert AllowIllegalOps || LInfo.

Also to answer one of the question, I believe we would want two different boolean:

  • Allow creating illegal ops or not
  • Legalize illegal ops or not
33 ↗(On Diff #127443)

Where should the legality checks be done?

Basically, we need a method that all the all the subclasses of GICombinerInfo can call for this.

Maybe something around those lines:
combine(...) {
combineImpl(...) the thing that gets override
/ check legality stuff
}

include/llvm/CodeGen/GlobalISel/GMIPatternMatch.h
6 ↗(On Diff #127443)

The header looks weird

include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
262 ↗(On Diff #127443)

Unrelated changes?

include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h
35 ↗(On Diff #127443)

The relationship between PreLegalizeCombiner and GICombiner looks strange to me.
In particular, before looking at the actual implementation of runOnMachineFunction, there is nothing showing that this pass uses GICombiner.

I would have expected something like GICombiner is a pass and PreLegalizeCombiner inherit from it and only set its custom PreLegalizeCombinerInfo thing.

41 ↗(On Diff #127443)

Unless I missed it, there is no implementation for that.

lib/CodeGen/GlobalISel/CMakeLists.txt
6

Sort alphabetically

One more comment: Do you have an idea how we could make sure that the combiners are making progress?
Basically, I would like we think about two cases that bit us in the past:

  1. Two combines undo each others
  2. Two combines keep piling up (like expand op1 into op2 & op3 and expand op2 into op1 & op3 would keep having the IR growing)

Thanks Quentin. Will update the patch with the comments shortly.
Regarding tracking progress, I don't have a really good solution - I suspect it's possible to detect loops when every transformation is written in tablegen, but with mixing in C++, a naive approach would be to issue some diagnostic/warning after some large number of iterations in GICombiner.

include/llvm/CodeGen/GlobalISel/GICombiner.h
12 ↗(On Diff #127443)

Thanks. Will do.

24 ↗(On Diff #127443)

Makes sense. Will update.

31 ↗(On Diff #127443)

I was wondering if the combiner info would have some state stored (possibly), and wanted only lvalue references (so the CombinerPass would create the Info on stack/heap and pass it in instead of passing in references to temporaries). Alternatively, the info could be copied (or passed in via unique_ptr.
The TPC is not currently used - making it const is definitely correct. Not really sure about if it can be null to make it reference.

include/llvm/CodeGen/GlobalISel/GICombinerHelper.h
1 ↗(On Diff #127443)

Will Update.

29 ↗(On Diff #127443)

Good point. We can remove the MRI.

34 ↗(On Diff #127443)

Yes - in the implementation, it does an early return if it's not a copy. You're right in that we need to document the expected behavior.

36 ↗(On Diff #127443)

the boolean return value is currently only used by the Combiner to track whether any change has been made so it needs to iterate again. It's also used to indicate if the pass made any changes.
Also, returning true (like the legalizer/selector) assumes that the instruction has been taken care of, and it's not guaranteed that the instruction is still alive.
Yes - documentation here would make it clear. Will do that.

include/llvm/CodeGen/GlobalISel/GICombinerInfo.h
28 ↗(On Diff #127443)

Yup - the assert makes sense here.Will update.

The two different boolean approaches also make sense.

33 ↗(On Diff #127443)

That sounds like a good approach.
Alternatively/Additionally, we can document that, the users should call some API to replace instructions (say CombineTo(FromMI, ToMI)) - where we can check before the original instruction is deleted, we can check the legality of the new instruction we're creating before replacing them. Of course, theres no way to force this upon the targets.

include/llvm/CodeGen/GlobalISel/GMIPatternMatch.h
6 ↗(On Diff #127443)

Will fix that.

include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
262 ↗(On Diff #127443)

Might make sense to split this in a separate patch.
These build methods are used in the unit test for the patternMatchers.

include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h
35 ↗(On Diff #127443)

I went with the "has a" relationship with the GICombiner. Pretty much what the GICombiner does right now is the driving of the combines which felt like it belonged in a utility. Right now, there should be little/no difference to switch to each pass inheriting the GICombiner and I can do that if required.

41 ↗(On Diff #127443)

This doesn't belong here (it's in the GICombiner). Good catch. Will remove that.

lib/CodeGen/GlobalISel/CMakeLists.txt
6

Thanks. Will do.

hfinkel added inline comments.
include/llvm/CodeGen/GlobalISel/GMIPatternMatch.h
163 ↗(On Diff #127443)

Why is this named gi_match? Doesn't it apply to any MI as well?

include/llvm/CodeGen/GlobalISel/GMIPatternMatch.h
163 ↗(On Diff #127443)

Hi Hal,

The match function above has been named gi_match as I was not sure if there is interest in using this beyond GlobalISel. We can definitely have it named match (and perhaps remove the GI prefix from all the matchers as well).

hfinkel added inline comments.Dec 20 2017, 11:12 AM
include/llvm/CodeGen/GlobalISel/GMIPatternMatch.h
163 ↗(On Diff #127443)

I think it would be great to be able to use these on SSA MI as well (but then having the 'gi' prefix will look odd).

include/llvm/CodeGen/GlobalISel/GMIPatternMatch.h
163 ↗(On Diff #127443)

Thanks for the feedback. I'll change the name to match.

I think it would be helpful to articulate here or in another patch/discussion the rationale for having a pre-legalize pass. While targets can of course do as they see fit, we should have some documentation on the source explaining what kind things are expected to be done there, and what kind of things are suggested to be done later. I.e. we can do some early canonicalization, early simplification to save compile time etc.

include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h
35 ↗(On Diff #127443)

I agree with Quentin that it seems having a generic GICombiner to inherit from seems clearer from a design point of view.

I will update the patch shortly with the feedback.

With respect to why I put up the PreLegalizationCombine pass, the main reason was to show a proof of concept pass to use the interfaces.
With respect to what to put in, I've seen lots of terrible code produced by the IRTranslator. Some cases that I saw were
getelementptr expanding to MUL with 1.

Sequence of LLVM_IR instructions building a 4 x vector with 4 insertelements result in IRTranslator producing 4 INSERT_VECTOR_ELT where as we can produce 1 MERGE_VALUES.
I can imagine similar cases of insertvectorelements followed by extractvectorelements which can probably be directly simplified as copies.

The other reason could be for canonicalization for targets (for eg, selects -> select_cc etc).

While these are not strictly necessary for correctness(also some of these probably belong in instcombine), I think it will simplify a lot of the code to be handled in subsequent passes, as well as compile time.

include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h
35 ↗(On Diff #127443)

Thanks. I will update that shortly.

aditya_nandakumar marked 9 inline comments as done.Dec 25 2017, 2:44 PM

Updated with feedback.

Updated based on feedback.

Missed removal of an un-used function.

include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h
35 ↗(On Diff #127443)

The one downside I see to making it based on inheritance is that, things like MachineFunction is not available during creation of the CombinerInfo (pass construction). Passes would need to initialize the legalizerInfo during the runOnMachineFunction call instead if we go this route.

GICombinerHelper - this contains transformations that are common to all targets. Targets can pick and choose which transformations (at function/opcode granularity) each pass uses via configuring a GICombinerInfo.

Could you clarify how you see this class growing in the long run? I'm a bit worried that we will see a proliferation of functions because a few targets (or just one) needed to do something a little different.

To use an existing example from SelectionDAG, suppose there is a tryCombineSelect() that contains target independent combines (select (not A), B, C) -> (select A, C, B) and similar. Now suppose a group of targets want to take advantage of the fact that comparisons produce 0/1 and add rules like (select A, 1, 0) -> A. How would this be integrated? I can see two main ways:

  • We can add the new rules to the existing tryCombineSelect() with appropriate guards.
  • We can break up tryCombineSelect() into pieces and have targets stick the pieces together the way they want. tryCombineSelect() is renamed to tryCombineSelect_AllTargets(), a new tryCombineSelect_CmpUsesZeroOrOne() is added.

The former is simpler but it doesn't allow targets to eliminate code they don't need which is something we've generally been trying to do and we don't seem to have a way to provide the values for the guards in the current design. The latter does support eliminating code but is ugly and gets worse as we'll soon see.

Now suppose other targets want to take advantage of their comparisons producing 0/-1 to do the same thing. Similar to before, we can either:

  • Add the new rules to tryCombineSelect() with appropriate guards.
  • Add a tryCombineSelect_CmpUsesZeroOrMinusOne().

Now suppose an awkward target comes along and wants to take advantage of scalar compares producing 0/1 and vector compares producing 0/-1. The code for both already exists in CombinerHelper so we don't really want to re-implement it, but we also don't really want to split tryCombineSelect_CmpUsesZeroOrOne() into tryCombineSelect_CmpUsesZeroOrOne_scalar() and tryCombineSelect_CmpUsesZeroOrOne_vector() as well as tryCombineSelect_CmpUsesZeroOrMinusOne() into tryCombineSelect_CmpUsesZeroOrMinusOne_scalar() and tryCombineSelect_CmpUsesZeroOrMinusOne_vector() since the proliferation of functions is starting to get unwieldy. We have five functions to handle various selects already and I haven't accounted for floating point yet.

What are your thoughts on how CombinerHelper will grow?

dsanders added inline comments.Jan 2 2018, 6:05 PM
include/llvm/CodeGen/GlobalISel/MIPatternMatch.h
13–14 ↗(On Diff #128147)

Just a little nit: These guards should follow the convention

include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h
35 ↗(On Diff #127443)

You should be able to retrieve it from MI.getParent().getParent().getSubtarget().getLegalizerInfo() or something like that

GICombinerHelper - this contains transformations that are common to all targets. Targets can pick and choose which transformations (at function/opcode granularity) each pass uses via configuring a GICombinerInfo.

Could you clarify how you see this class growing in the long run? I'm a bit worried that we will see a proliferation of functions because a few targets (or just one) needed to do something a little different.

To use an existing example from SelectionDAG, suppose there is a tryCombineSelect() that contains target independent combines (select (not A), B, C) -> (select A, C, B) and similar. Now suppose a group of targets want to take advantage of the fact that comparisons produce 0/1 and add rules like (select A, 1, 0) -> A. How would this be integrated? I can see two main ways:

  • We can add the new rules to the existing tryCombineSelect() with appropriate guards.
  • We can break up tryCombineSelect() into pieces and have targets stick the pieces together the way they want. tryCombineSelect() is renamed to tryCombineSelect_AllTargets(), a new tryCombineSelect_CmpUsesZeroOrOne() is added.

The former is simpler but it doesn't allow targets to eliminate code they don't need which is something we've generally been trying to do and we don't seem to have a way to provide the values for the guards in the current design. The latter does support eliminating code but is ugly and gets worse as we'll soon see.

Now suppose other targets want to take advantage of their comparisons producing 0/-1 to do the same thing. Similar to before, we can either:

  • Add the new rules to tryCombineSelect() with appropriate guards.
  • Add a tryCombineSelect_CmpUsesZeroOrMinusOne().

Now suppose an awkward target comes along and wants to take advantage of scalar compares producing 0/1 and vector compares producing 0/-1. The code for both already exists in CombinerHelper so we don't really want to re-implement it, but we also don't really want to split tryCombineSelect_CmpUsesZeroOrOne() into tryCombineSelect_CmpUsesZeroOrOne_scalar() and tryCombineSelect_CmpUsesZeroOrOne_vector() as well as tryCombineSelect_CmpUsesZeroOrMinusOne() into tryCombineSelect_CmpUsesZeroOrMinusOne_scalar() and tryCombineSelect_CmpUsesZeroOrMinusOne_vector() since the proliferation of functions is starting to get unwieldy. We have five functions to handle various selects already and I haven't accounted for floating point yet.

What are your thoughts on how CombinerHelper will grow?

With this approach, I expect adding target specific rules to be fairly easy (Targets can extend/inherit GICombinerHelper) and add specific overrides for the generic tryVisitOpcode.
Removing rules on the other hand is a little difficult. If the target sees a transformation that they don't want (which the generic helper does), they have the option to match that specific pattern and early return false. This works, but I don't think this is elegant design wise.
Regarding guards vs splitting functions and chaining various pieces, I see pros and cons to both approaches. I would probably go with the latter and make the targets opt in to all the select transformations they want. If it indeed does go out of hand (how many such cases really exist?), then we probably need to revisit this part.

include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h
35 ↗(On Diff #127443)

The problem is, when do you initialize it? Do you check if it's initialized during each combine invocation? It seems wasteful to have to check every time (even if initialized).
The alternative is to have a setter that initializes the LegalizerInfo during runOnMachineFunction.

bogner added inline comments.Jan 9 2018, 2:31 PM
include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h
35 ↗(On Diff #127443)

FWIW I think the has-a relationship that Aditya's gone with is much simpler and clearer here.

The pass has the single responsibility of setting things up for the pass pipeline itself, while the GICombiner parts have their own single responsibility of doing the combines themselves. The alternative approach with inheritance will muddy up these concepts and make it harder to tell where the lines between concepts are.

Mostly looks good to me.
We probably want to make two separate patches:

  • One with the pattern matching capabilities
  • One with the new pass
include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h
35 ↗(On Diff #127443)

That's a good point. Given how simple the pass itself is, it is fine not to involve inheritance at this point.

Split the PatternMatcher part to https://reviews.llvm.org/D42439
Updated based on Justin, Quentin's feedback.

I think this is ok to progress now. We're happy with the overall design, although the pre-legalize pass is probably unnecessary at this moment, so leave that for another patch?

I think this is ok to progress now. We're happy with the overall design, although the pre-legalize pass is probably unnecessary at this moment, so leave that for another patch?

Sure. I can split that up.

As per Amara's feedback, removed the prelegalizeCombiner from this review. I can put it up in a separate review as an example in tree for how to use the framework.

aemerson accepted this revision.Jan 24 2018, 2:50 PM

LGTM with some minor comment fixes.

include/llvm/CodeGen/GlobalISel/CombinerHelper.h
13

80 cols?

include/llvm/CodeGen/GlobalISel/CombinerInfo.h
11

Superfluous '//'.

lib/CodeGen/GlobalISel/Combiner.cpp
11

Same here.

lib/CodeGen/GlobalISel/CombinerHelper.cpp
40

I think we're better off without this comment.

This revision is now accepted and ready to land.Jan 24 2018, 2:50 PM
aditya_nandakumar marked 4 inline comments as done.Jan 24 2018, 3:39 PM

Thanks @aemerson . Fixed the nits you mentioned.

Committed in r323392