This is an archive of the discontinued LLVM Phabricator instance.

AArch64CollectLOH: Rewrite as block-local analysis.
ClosedPublic

Authored by MatzeB on Dec 1 2016, 6:22 PM.

Details

Summary

Previously this pass was using up to 5% compiletime in some cases which
is a bit much for what it is doing. The pass featured a full blown
dataflow analysis which in the default configuration was restricted to a
single block.

This rewrites the pass under the assumption that we only ever work on a
single block. It now works in a single pass just maintaining a small state
machines per tracked physreg. This makes the pass 5-10x faster.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 80013.Dec 1 2016, 6:22 PM
MatzeB retitled this revision from to AArch64CollectLOH: Rewrite as block-local analysis..
MatzeB updated this object.
MatzeB added a reviewer: qcolombet.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: llvm-commits.
qcolombet accepted this revision.Dec 2 2016, 10:49 AM
qcolombet edited edge metadata.

Hi Matthias,

This looks indeed much simpler! That helps to narrow down the supported cases :).
LGTM, with a couple of nitpicks.
Also, the pass does not have any DEBUG statement now AFAICT. Could you add some?

Thanks,
-Quentin

lib/Target/AArch64/AArch64CollectLOH.cpp
274 ↗(On Diff #80013)

Given how the code is structured, i.e., this definition and all the helper functions before the main algorithm, I would add a comment saying that this structure will be populated via a bottom up traversal of the basic blocks.
Then, a LOH will be recorded when we reach an ADRP with a candidate chain (or chains when we have the ADRP_ADRP case on top of the other).

278 ↗(On Diff #80013)

I wouldn't bother defining bitfield size.

328 ↗(On Diff #80013)

Typo -> s/an/a

355 ↗(On Diff #80013)

I'd also assert that the operand is a GOT entry.

505 ↗(On Diff #80013)

Shouldn't we have a default case to avoid warnings?

test/CodeGen/AArch64/arm64-collect-loh.ll
637 ↗(On Diff #80013)

Why is this not on the next line anymore?

670 ↗(On Diff #80013)

There shouldn't be any nondeterminism in the output, so I would rather stick to whatever order you get now.

This revision is now accepted and ready to land.Dec 2 2016, 10:49 AM
MatzeB marked 7 inline comments as done.Dec 2 2016, 4:59 PM

Thanks for the review. I added some debug statements and spend some time creating a .mir test for all situations happening in the code (and fixed a bug discovered while doing so).

lib/Target/AArch64/AArch64CollectLOH.cpp
328 ↗(On Diff #80013)

I imagine this as "an 'el' 'oh' 'haitch'"

355 ↗(On Diff #80013)

Good point.

505 ↗(On Diff #80013)

We're switching over unsigned Opcode and don't get a warning.

test/CodeGen/AArch64/arm64-collect-loh.ll
637 ↗(On Diff #80013)

This example contained a perfectly fine LOH opportunity that wasn't catched before (probably because the old algorithm bailed out on the double q reg and didn't resume properly on the LDR address). I updated the test to make it apparent.

670 ↗(On Diff #80013)

ok.

This revision was automatically updated to reflect the committed changes.