[PATCH] D105069: [GlobalISel] Add re-association combine for G_PTR_ADD to allow better addressing mode usage.

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1677
+  // to go beyond the bounds of our legal addressing modes.
+  if (!MRI.hasOneNonDBGUse(Add2))
+    return false;
----------------
arsenm wrote:
> Check isLegalAddressingMode, or some similar hook?
I don't think it's necessary for this. We should avoid doing this combine no matter what with multiple users since doing so doesn't give any benefit since the def can't be deleted.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4040-4041
+    MachineInstr &PtrAdd) {
+  if (PtrAdd.getOpcode() != TargetOpcode::G_PTR_ADD)
+    return false;
+
----------------
arsenm wrote:
> This should be impossible?
I'll change this to an assert for now.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4044
+  Register Src1Reg = PtrAdd.getOperand(1).getReg();
+  MachineInstr *Src1Def = MRI.getVRegDef(Src1Reg);
+  if (Src1Def->getOpcode() != TargetOpcode::G_PTR_ADD)
----------------
paquette wrote:
> `getOpcodeDef`?
+1


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4055
+  auto C2 = getConstantVRegVal(Src2Reg, MRI);
+  if (!C1 || !C2)
+    return false;
----------------
paquette wrote:
> maybe minorly faster
> 
> ```
> auto C1 = getConstantVRegVal(Src1Def->getOperand(2).getReg(), MRI);
> if (!C1)
>   return false
> 
> auto C2 = getConstantVRegVal(Src2Reg, MRI);
> if (!C2)
>   return false;
> ```
> 
> You could probably pull in some of the C1/C2APIntVal stuff below into there too, although that probably matters less.
> 
> 
Sure.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4069-4070
+  for (auto &UseMI : MRI.use_nodbg_instructions(Src1Reg)) {
+    // This combine may end up running before ptrtoint/inttoptr combines
+    // manage to eliminate redundant conversions, so try to look through them.
+    MachineInstr *ConvUseMI = &UseMI;
----------------
arsenm wrote:
> Don't we retry combines like the DAG? This seems like a bigger structural problem to solve rather than specially treating this
We do, but this is a heuristic and not a combine. If we can't prove that we break an addressing mode (by detecting a memory-operation user), then we return false, and so the reassociation is done. Once it's done, we don't revisit the decision and undo it later, so we need to have a best effort to catch these cases here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105069