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

[Aditya Nandakumar via llvm-commits]

Hi Gerolf,

I anticipate the PreLegalizeCombiner to contain combines that are purely for performance - ie it can significantly reduce the amount of work that needs to be done by the legalizer. I don’t anticipate any correctness combines in the PreLegalizeCombiner(Though Targets are free to do as they please).

Also, the PreLegalizeCombiner was an example (maybe a poor one?) of how a pass would look like.

Thanks
Aditya

> On Jan 2, 2018, at 7:29 AM, Gerolf Hoflehner <ghoflehner at apple.com> wrote:
> 
> I’m still a bit confused by a PreLegalizeCombiner. It mixed optimizations and correctness. I would expect a pre-legalize pass to apply only transformations necessary for the legalizer to work correctly.
> 
> -Gerolf
> 
> 
>> On Dec 20, 2017, at 4:23 PM, Amara Emerson via Phabricator <reviews at reviews.llvm.org> wrote:
>> 
>> aemerson added a comment.
>> 
>> 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.
>> 
>> 
>> 
>> ================
>> Comment at: include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h:35
>> +
>> +class PreLegalizeCombiner : public MachineFunctionPass {
>> +public:
>> ----------------
>> aditya_nandakumar wrote:
>>> qcolombet wrote:
>>>> 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.
>>> 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.
>> I agree with Quentin that it seems having a generic GICombiner to inherit from seems clearer from a design point of view.
>> 
>> 
>> https://reviews.llvm.org/D41373
>> 
>> 
>> 
>