[PATCH] D41373: [GISel][RFC]: GlobalISel Combiner prototype

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added a comment.

In https://reviews.llvm.org/D41373#966318, @dsanders wrote:

> > 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.


https://reviews.llvm.org/D41373