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

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:459-460
+  Addr = MI.getOperand(1).getReg();
+  MachineInstr *AddrDef = MRI.getUniqueVRegDef(Addr);
+  if (!AddrDef || AddrDef->getOpcode() != TargetOpcode::G_GEP ||
+      MRI.hasOneUse(Addr))
----------------
I think it's safe to use `getOpcodeDef` from Utils here. It looks through copies but I don't think that's an issue here.

Then you can just do this to get a G_GEP or null:

```
MachineInstr *AddrDef = getOpcodeDef(TargetOpcode::G_GEP, Addr, MRI);
```


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:461
+  if (!AddrDef || AddrDef->getOpcode() != TargetOpcode::G_GEP ||
+      MRI.hasOneUse(Addr))
+    return false;
----------------
Should this be `!MRI.hasOneUse(Addr)`?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:475
+
+  MachineInstr *BaseDef = MRI.getUniqueVRegDef(Base);
+  if (BaseDef->getOpcode() == TargetOpcode::G_FRAME_INDEX) {
----------------
I think you can probably use `getDefIgnoringCopies` here?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:488-491
+    // We're expecting one use of Addr in MI, but it could also be the
+    // value stored, which isn't actually dominated by the instruction.
+    if (MI.getOperand(0).getReg() == Addr)
+      return false;
----------------
Should this have some debug output for consistency?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:515-517
+  bool IsPre = false;
+  if (findPreIndexCandidate(MI, Addr, Base, Offset))
+    IsPre = true;
----------------
`bool IsPre = findPreIndexCandidate(MI, Addr, Base, Offset)` would be a bit simpler I guess?

Then you could just go

```
if (!IsPre && !findPostIndexCandidate(MI, Addr, Base, Offset))
  return false;

```

But meh


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:551-552
+
+  MIB.addReg(Base);
+  MIB.addReg(Offset);
+  MIB.addImm(IsPre);
----------------
Any reason this isn't `MIB.addUse(Base)` and `MIB.addUse(Offset)`?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:555
+  MI.eraseFromParent();
+  AddrDef.eraseFromParent();
+
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66287