[PATCH] D66287: GlobalISel: add combiner for indexed loads and stores
Jessica Paquette via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 15 09:31:41 PDT 2019
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
More information about the llvm-commits
mailing list