This is an archive of the discontinued LLVM Phabricator instance.

AArch64: Enable post-ra liveness updates
ClosedPublic

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

Details

Summary

Fix the situations in which liveness information was not properly maintained/updated.

This is a prerequesite to have the rewritten Macho CollectLOH pass work reliably.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 80705.Dec 7 2016, 6:22 PM
MatzeB retitled this revision from to AArch64: Enable post-ra liveness updates.
MatzeB updated this object.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: llvm-commits.
mcrosier resigned from this revision.Dec 15 2016, 8:37 AM
mcrosier edited reviewers, added: gberry; removed: mcrosier.
gberry added inline comments.Dec 15 2016, 12:28 PM
lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
901

Typo? "LSR" -> "LR"

902

Typo: remove "have"

907

Is the reason you need LR to be undef here because we haven't added the corresponding def of LR to the function at this point?

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
835

This is conservative, right? Ideally you would copy the kill flags over to the last new instruction that uses StRt?

test/CodeGen/AArch64/ldst-opt-dbg-limit.mir
128

I don't understand why you removed the kills here.

test/CodeGen/AArch64/loh.mir
57 ↗(On Diff #80705)

Maybe add "because of the above clobber of x9" to this comment?

103 ↗(On Diff #80705)

I don't follow this change of registers.

173 ↗(On Diff #80705)

I don't follow this either. It seems like you are changing what is being tested.

MatzeB marked 5 inline comments as done.Dec 15 2016, 3:10 PM

Thanks for the review!

lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
901

Yes a typo, but I wanted to write CSR (Callee Saved Register)

902

yep

907

To give some context: I believe Ret_ReallyLR is hack to hide the fact that LR is used from the register allocator, because in old versions of the code simply reading a register would trigger the callee saved register handling to unnecessarily spill and restore. I actually fixed that bad behavior a year ago, however trying to get rid of the Ret_ReallyLR hack in this commit put me a lot deeper into the rabid hole than I wanted to go when I tried it so I left that for another day in the end.

The reason for the undef flag is that LR now looks unused (at the end of the function), so in some instances we would produce operands like LR<kill> in the middle of the function indicating liveness analysis that the value gets killed there. This effectively is invalid liveness information but without trackLivenessAfterRegAlloc() enabled nobody would access liveness information late in the compiler so the verifier would not check it. So to appease the verifier I add an undef flag here, the callee save register handling will always ensure anyway that LR is restored at the end, no matter if the return has an actual LR use or not.
(Yes it feels like a hack but we are forced to do it as long as the Ret_ReallyLR hack is around).

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
835

Yes it is conservative. But kill flags themselfes are considered conservative nowadays anyway, because updating them correctly turned out to be a maintenance nightmare so nowadays we usually go with the simpler fix and simply remove them.
(Getting completely rid of kill flags is on the long term TODO list, but that is another longer story, parts of it can be seen in my patch series to have the regscavenger/prologepilog inserter walk backwards instead of forwards which is somewhat staled on hard to debug bot failure)

test/CodeGen/AArch64/loh.mir
57 ↗(On Diff #80705)

ok

103 ↗(On Diff #80705)

Oh this should have ended up in a separate commit. It fixes a bug in the test not a problem related to the trackLivenessAfterRegAlloc().

173 ↗(On Diff #80705)

Yep another fix for a bug in the test that should have been in a separate commit.

MatzeB updated this revision to Diff 81673.Dec 15 2016, 3:11 PM
MatzeB marked 4 inline comments as done.

Update as discussed. Note that the loh mir test is no longer part of this patch as it is currently reverted from ToT.

gberry accepted this revision.Dec 16 2016, 1:29 PM
gberry edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 16 2016, 1:29 PM
This revision was automatically updated to reflect the committed changes.