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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

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


https://reviews.llvm.org/D41373