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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

Thanks Amara

> I have some questions inline, but overall it seems that we don't keep track of how many of each use
>  is a particular kind of extend. Could we have situations where overall we increase code size due to,
>  for example, preferring a sign extend even if we have multiple zero-extending users? This might not
>  be worth tackling in a first pass at this if it's a rare case though.

That's a good point. Yes, that can happen at the moment.

I can see two cases (and mixtures of the two). The first is multiple zexts to the same type. In this case, we would generate one trunc/zext pair for each use. CSE ought to fix it later (once we enable that) but it would be better to CSE it up front and avoid paying the cost of allocating+processing the redundant instructions. A simple map of opcode and LLT to vreg while emitting the trunc/zext's ought to do the trick there. The other is the case where there's zexts to multiple types. In this case, picking the sext and trunc/zext is still a win as it eliminates 1-2 instructions whereas whichever zext we pick can eliminate at most 1 since all but one zext still needs to emit an instruction. There's target-specific special cases though. For example, Mips sext from 32-bit to >32-bit is free (because it was actually done by the trunc or the gpr32 instruction that def'd it) so picking that particular sext is worse than picking any zext.



================
Comment at: include/llvm/CodeGen/GlobalISel/CombinerHelper.h:34
+  void eraseInstr(MachineInstr &MI);
+  void scheduleForVisit(MachineInstr &MI);
 
----------------
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


================
Comment at: lib/CodeGen/GlobalISel/Combiner.cpp:29
+namespace {
+class WorkListMaintainer : public CombinerChangeObserver {
+  GISelWorkList<512> &WorkList;
----------------
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.


================
Comment at: lib/CodeGen/GlobalISel/Combiner.cpp:33
+public:
+  WorkListMaintainer(GISelWorkList<512> &WorkList) : WorkList(WorkList) {}
+  virtual ~WorkListMaintainer() {}
----------------
aemerson wrote:
> Should we have a specialization for `GISelWorklist<512>`, and then not repeat the 512 in multiple places?
Yes, let's add an alias for it as Combiner::WorkListTy


================
Comment at: lib/CodeGen/GlobalISel/Combiner.cpp:37
+  void erasedInstr(MachineInstr &MI) override {
+    errs() << "Erased: ";
+    MI.print(errs());
----------------
aemerson wrote:
> Delete or DEBUG()?
Oops. That was some temporary debugging code. It's useful for tracing the combines though so let's make it DEBUG()/dbgs()


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:55
+  };
+  const auto ChoosePreferredScalar =
+      [](PreferredTuple &A, const LLT &TyB, unsigned ExtendOpcodeB,
----------------
aemerson wrote:
> Can we promote this lambda into a helper function? From the name and lack of header doc it's unclear what it's supposed to do with the params on first reading.
Sure.


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:64
+           ExtendOpcodeB == TargetOpcode::G_ZEXT ||
+           ExtendOpcodeB == TargetOpcode::G_ANYEXT))
+        return {TyB, ExtendOpcodeB, InstrB};
----------------
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


Repository:
  rL LLVM

https://reviews.llvm.org/D45543