This is an archive of the discontinued LLVM Phabricator instance.

AArch64: Schedule DeadRegisterDefinitionsPass before regalloc.
ClosedPublic

Authored by MatzeB on Oct 28 2016, 7:17 PM.

Details

Summary

Doing this before register allocation reduces register pressure as we do
not even have to allocate a register for those dead definitions.

(Requires D26106 to avoid some accidental regressions).

I am running tests on the effects now and add a comment here.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 76287.Oct 28 2016, 7:17 PM
MatzeB retitled this revision from to AArch64: Schedule DeadRegisterDefinitionsPass before regalloc..
MatzeB updated this object.
MatzeB added a subscriber: llvm-commits.
mcrosier resigned from this revision.Nov 1 2016, 1:38 PM
mcrosier edited reviewers, added: gberry; removed: mcrosier.
MatzeB added a comment.Nov 1 2016, 4:09 PM

Turns out you also need D26221 to avoid some regressions with this.

gberry edited edge metadata.Nov 8 2016, 12:43 PM

This mostly looks good to me, though I have a few questions below.

lib/Target/AArch64/AArch64DeadRegisterDefinitionsPass.cpp
92 ↗(On Diff #76287)

Perhaps you could add an assert here checking this?

96 ↗(On Diff #76287)

Is this use_nodbg_empty check necessary? Can we end up with virtual registers marked dead with dbg uses?

148 ↗(On Diff #76287)

Perhaps you should commit this bug fix (local Changed shadowing the member) separately?

MatzeB added inline comments.Nov 8 2016, 1:00 PM
lib/Target/AArch64/AArch64DeadRegisterDefinitionsPass.cpp
92 ↗(On Diff #76287)

I'd rather not, as it is technically legal to do so. This is merely an explanation why I did not add code to handle the physreg case; and I'd rather not add that code as it would be unused and hence untested today.

96 ↗(On Diff #76287)

This is not about debug values (that function ignores any debug uses), but a way to detect dead definitions that do not have the Dead flag set.

I encountered several instances of dead defs without a dead flag set. I am not entirely sure whether the missing Dead flags are a bug or whether they need not be exact pre-regalloc. In any case you can see that DeadMachineInstructionElim does the same check to determine whether a def is dead.

148 ↗(On Diff #76287)

ok.

qcolombet accepted this revision.Nov 15 2016, 3:50 PM
qcolombet edited edge metadata.

Hi Matthias,

LGTM.
One comment, could you add a test case where this save spill?
I guess it requires a bit of work given we need to expose case with spill, but if you can come up with something reasonably small, go for it!
Otherwise, I guess it is already well covered by the exiting tests.

Cheers,
-Quentin

This revision is now accepted and ready to land.Nov 15 2016, 3:50 PM

Hi Matthias,

LGTM.
One comment, could you add a test case where this save spill?

sure.

This revision was automatically updated to reflect the committed changes.