[PATCH] D45543: [globalisel] Add a combiner helpers for extending loads and use them in a pre-legalize combiner for AArch64

[Amara Emerson via Phabricator via llvm-commits]

aemerson requested changes to this revision.
aemerson added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/CodeGen/GlobalISel/CombinerHelper.h:34
+  void eraseInstr(MachineInstr &MI);
+  void scheduleForVisit(MachineInstr &MI);
 
----------------
dsanders wrote:
> aemerson wrote:
> > There doesn't seem to be a user of this.
> That's right. It's not needed for this particular combine but there should be some that need it in future. I could drop it from this patch
Ok, as long as we're aware let's keep it in.


================
Comment at: lib/CodeGen/GlobalISel/Combiner.cpp:29
+namespace {
+class WorkListMaintainer : public CombinerChangeObserver {
+  GISelWorkList<512> &WorkList;
----------------
dsanders wrote:
> aemerson wrote:
> > What's the purpose of this? Currently it just wraps around the underlying worklist. Debugging messages only?
> The intent behind CombinerChangeObserver is to inform the combiner pass that certain events happened in CombinerHelper. The main one's I'm expecting to end up with are instruction creation and deletion. I can also see instruction modification events here in future. I don't think scheduling for revisit should be in CombinerChangeObserver though. I think that should be determined by the combiner pass in response to these events. It should also be derived from the implemented combines in some way to avoid the issue in DAGCombine where combines are sometimes missed because one rule failed to schedule the right node (e.g. because the root wasn't directly connected to the modified nodes in the case combines that cover several nodes).
> 
> The reason for having CombinerChangeObserver subclasses rather than just implementing it directly is so that CombinerHelper is usable by lots of different kinds of Combiner passes. We might have different implementations for `O0` - `O3`, or a specific strategy might work better for one particular target, or maybe the pass isn't a combiner at all and just wants to borrow a couple combines. Each implementation would provide a CombinerChangeObserver subclass that glues CombinerHelper into its implementation.
> 
> This particular implementation ought to grow a mechanism to limit the run-time of the pass. Exactly how that should work will need some more thinking about but as an example we could track how deeply recursed the combiner is (i.e. how many combine rules triggered to create it) and decline to schedule instructions beyond a certain point. For `O1` combines might be limited to the first generation of new/modified instructions, for `O2` maybe the first 3 generations, and `O3` might be unlimited.
Ok thanks. I think it would be beneficial to have a comment at the definition (can be your reply here summarised).


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:64
+           ExtendOpcodeB == TargetOpcode::G_ZEXT ||
+           ExtendOpcodeB == TargetOpcode::G_ANYEXT))
+        return {TyB, ExtendOpcodeB, InstrB};
----------------
dsanders wrote:
> aemerson wrote:
> > What else could ExtendOpcodeB be at this point? If nothing else, should be an assert?
> Anything. There's no guarantee that the use of a load is an extend. We ignore that when selecting a preferred use and deal with the non-extends by inserting a trunc (which is free for most targets) later
Then perhaps change the name to something more accurate, like "UseOpcode"?


Repository:
  rL LLVM

https://reviews.llvm.org/D45543