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

[Jessica Paquette via Phabricator via llvm-commits]

paquette accepted this revision.
paquette added a comment.
This revision is now accepted and ready to land.

Just a couple nits and then this looks fine to me.



================
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;
----------------
I think a bunch of the matching logic here could be done with `mi_match`?

something like this?

```
if (!mi_match(Add2, MRI, m_OneNonDBGUse(m_GPtrAdd(m_Reg(Base), m_ICst(Imm2)))))
   return false;
```

although if looking through the constants is important, then maybe this won't work.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4065
+  const APInt CombinedValueIntVal = C1APIntVal + C2APIntVal;
+  if (CombinedValueIntVal.getBitWidth() > 64)
+    return false;
----------------
I don't think the testcases cover this and the

```
 if (C1APIntVal.getBitWidth() > 64 || C2APIntVal.getBitWidth() > 64)
```

case?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4080
+    }
+    auto LoadStore = ConvUseMI->getOpcode() == TargetOpcode::G_LOAD ||
+                     ConvUseMI->getOpcode() == TargetOpcode::G_STORE;
----------------
might as well make ConvUseMI->getOpcode() a variable


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4082
+                     ConvUseMI->getOpcode() == TargetOpcode::G_STORE;
+    if (LoadStore) {
+      // Is x[offset2] already not a legal addressing mode? If so then
----------------
maybe slightly easier to read if you reduce the indentation level here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105069