[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 added a comment.

Hi Daniel, sorry for the delay.

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.



================
Comment at: include/llvm/CodeGen/GlobalISel/CombinerHelper.h:34
+  void eraseInstr(MachineInstr &MI);
+  void scheduleForVisit(MachineInstr &MI);
 
----------------
There doesn't seem to be a user of this.


================
Comment at: lib/CodeGen/GlobalISel/Combiner.cpp:29
+namespace {
+class WorkListMaintainer : public CombinerChangeObserver {
+  GISelWorkList<512> &WorkList;
----------------
What's the purpose of this? Currently it just wraps around the underlying worklist. Debugging messages only?


================
Comment at: lib/CodeGen/GlobalISel/Combiner.cpp:33
+public:
+  WorkListMaintainer(GISelWorkList<512> &WorkList) : WorkList(WorkList) {}
+  virtual ~WorkListMaintainer() {}
----------------
Should we have a specialization for `GISelWorklist<512>`, and then not repeat the 512 in multiple places?


================
Comment at: lib/CodeGen/GlobalISel/Combiner.cpp:37
+  void erasedInstr(MachineInstr &MI) override {
+    errs() << "Erased: ";
+    MI.print(errs());
----------------
Delete or DEBUG()?


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:55
+  };
+  const auto ChoosePreferredScalar =
+      [](PreferredTuple &A, const LLT &TyB, unsigned ExtendOpcodeB,
----------------
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.


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:64
+           ExtendOpcodeB == TargetOpcode::G_ZEXT ||
+           ExtendOpcodeB == TargetOpcode::G_ANYEXT))
+        return {TyB, ExtendOpcodeB, InstrB};
----------------
What else could ExtendOpcodeB be at this point? If nothing else, should be an assert?


Repository:
  rL LLVM

https://reviews.llvm.org/D45543