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

[Justin Bogner via Phabricator via llvm-commits]

bogner added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h:35
+
+class PreLegalizeCombiner : public MachineFunctionPass {
+public:
----------------
aditya_nandakumar wrote:
> dsanders wrote:
> > aditya_nandakumar wrote:
> > > aditya_nandakumar wrote:
> > > > 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.
> > > 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.
> > You should be able to retrieve it from MI.getParent().getParent().getSubtarget().getLegalizerInfo() or something like that
> 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.
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.



https://reviews.llvm.org/D41373