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

[Tim Northover via Phabricator via llvm-commits]

t.p.northover marked 12 inline comments as done.
t.p.northover added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:76
+  /// a single basic block.
+  bool dominates(MachineInstr &DefMI, MachineInstr &UseMI);
+
----------------
aditya_nandakumar wrote:
> We have an implementation in CSEMIRBuilder.cpp which also checks dominance. We should merge the implementations (or re-use).
That implementation looks more like `isPredecessor`, since it doesn't rely on MachineDominanceTree information. I suppose I could put a common implementation in Utils.h.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:461
+  if (!AddrDef || AddrDef->getOpcode() != TargetOpcode::G_GEP ||
+      MRI.hasOneUse(Addr))
+    return false;
----------------
paquette wrote:
> Should this be `!MRI.hasOneUse(Addr)`?
Nope, it's making sure there are uses *other* than the load/store we'll be making indexed (otherwise why bother).


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:555
+  MI.eraseFromParent();
+  AddrDef.eraseFromParent();
+
----------------
aditya_nandakumar wrote:
> 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.
Turns out it's needed because otherwise the transformation breaks SSA form.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66287