This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by aemerson on Jun 28 2021, 4:54 PM.

Details

Summary

We're trying to match a few pointer computation patterns here for re-association opportunities.

  1. Isolating a constant operand to be on the RHS, e.g.:

G_PTR_ADD(BASE, G_ADD(X, C)) -> G_PTR_ADD(G_PTR_ADD(BASE, X), C)

  1. Folding two constants in each sub-tree as long as such folding doesn't break a legal addressing mode.

G_PTR_ADD(G_PTR_ADD(BASE, C1), C2) -> G_PTR_ADD(BASE, C1+C2)

AArch64 code size improvements on CTMark with -Os:

Program              before  after   diff
pairlocalalign      251048  251044 -0.0%
consumer-typeset    421820  421812 -0.0%
kc                  431348  431320 -0.0%
SPASS               413404  413300 -0.0%
clamscan            384396  384220 -0.0%
tramp3d-v4          370640  370412 -0.1%
lencod              432096  431772 -0.1%
bullet              479400  478796 -0.1%
sqlite3             288504  288072 -0.1%
7zip-benchmark      573796  570768 -0.5%
Geomean difference                 -0.1%

Diff Detail

Event Timeline

aemerson created this revision.Jun 28 2021, 4:54 PM
aemerson requested review of this revision.Jun 28 2021, 4:54 PM
arsenm added inline comments.Jun 28 2021, 5:30 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1727

Check isLegalAddressingMode, or some similar hook?

4148–4149

This should be impossible?

4177–4178

Don't we retry combines like the DAG? This seems like a bigger structural problem to solve rather than specially treating this

paquette added inline comments.Jun 28 2021, 5:37 PM
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
537

maybe a doxygen comment?

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4152

getOpcodeDef?

4163

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.

aemerson added inline comments.Jun 28 2021, 5:55 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1727

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.

4148–4149

I'll change this to an assert for now.

4152

+1

4163

Sure.

4177–4178

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.

aemerson updated this revision to Diff 355280.Jun 29 2021, 9:54 AM

Address some comments.

paquette accepted this revision.Jul 2 2021, 10:32 AM

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

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1727

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.

4173

I don't think the testcases cover this and the

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

case?

4188

might as well make ConvUseMI->getOpcode() a variable

4190

maybe slightly easier to read if you reduce the indentation level here

This revision is now accepted and ready to land.Jul 2 2021, 10:32 AM
aemerson added inline comments.Jul 2 2021, 12:20 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1727

Yeah I think that wouldn't be NFC.

4173

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.

This revision was landed with ongoing or failed builds.Jul 2 2021, 12:31 PM
This revision was automatically updated to reflect the committed changes.
foad added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1727

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.

I don't understand. If Add2 is used by two ptr_adds, and both of them can be folded to give legal addresses, then we should do that and delete Add2. We are seeing some cases downstream where that is no longer happening because of this check.

aemerson added inline comments.Jul 8 2021, 8:20 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1727

Ok, fair enough. I’ll look into that case today.

craig.topper added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4171

sorry to dig up an old patch, but I think this should be checking the users of PtrAdd. See https://reviews.llvm.org/D124644

Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 4:40 PM