[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;
----------------
paquette wrote:
> 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.
Yeah I think that wouldn't be NFC.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4065
+  const APInt CombinedValueIntVal = C1APIntVal + C2APIntVal;
+  if (CombinedValueIntVal.getBitWidth() > 64)
+    return false;
----------------
paquette wrote:
> I don't think the testcases cover this and the
> 
> ```
>  if (C1APIntVal.getBitWidth() > 64 || C2APIntVal.getBitWidth() > 64)
> ```
> 
> case?
Yep, and it seems it's not actually possible to trigger that at all. I'll remove this code and also put up a patch to do the same for the DAGCombiner counterpart on which this code was based.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105069