[PATCH] D66287: GlobalISel: add combiner for indexed loads and stores

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:76
+  /// a single basic block.
+  bool dominates(MachineInstr &DefMI, MachineInstr &UseMI);
+
----------------
We have an implementation in CSEMIRBuilder.cpp which also checks dominance. We should merge the implementations (or re-use).


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:86
+  /// legal and the surrounding code makes it useful.
+  bool tryCombineIndexedLoadStore(MachineInstr &MI);
+
----------------
Any chance you can include examples (with MIR transformation as a comment) on what this is supposed to do?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:555
+  MI.eraseFromParent();
+  AddrDef.eraseFromParent();
+
----------------
paquette wrote:
> Probably worth mentioning earlier in that our ultimate goal is to delete this.
> 
> Also, I don't *think* you need to actually delete `AddrDef`. It has no uses at this point so it should automatically be cleaned up.
> 
> (With that in mind, I guess you don't really have to check that `AddrDef` has a single use, although I'm not entirely sure if that would ever be beneficial.)
Like @paquette said, there's no need to explicitly delete AddrDef - when the combiner gets to that instruction, if it's dead it should remove it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66287/new/

https://reviews.llvm.org/D66287