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

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added a comment.

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.



================
Comment at: include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h:35
+
+class PreLegalizeCombiner : public MachineFunctionPass {
+public:
----------------
aemerson wrote:
> 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.
Thanks. I will update that shortly.


https://reviews.llvm.org/D41373