This is an archive of the discontinued LLVM Phabricator instance.

Replace at most one dead register with zero register on aarch64
ClosedPublic

Authored by yuyichao on Mar 27 2016, 8:59 PM.

Details

Summary

Fix https://llvm.org/bugs/show_bug.cgi?id=27081

As mentioned in my comment of the bug report, this seems to be fix the problem but it does make a few assumptions. This is currently assuming that no live dest can be zero register before this pass and there shouldn't be more than one dead register in other instructions.

I searched through the aarch64 instruction set manual and it seems that the load pair instructions are the only ones that can cause this trouble so maybe it is best to just check for those.

Diff Detail

Event Timeline

yuyichao updated this revision to Diff 51769.Mar 27 2016, 8:59 PM
yuyichao retitled this revision from to Replace at most one dead register with zero register on aarch64.
yuyichao updated this object.
yuyichao added a reviewer: t.p.northover.
yuyichao added a subscriber: llvm-commits.
t.p.northover edited edge metadata.Apr 1 2016, 12:17 PM

I think it would be better to check MI->definesRegister(AArch64::XZR) || MI->definesRegister(AArch64::WZR) and continue. It seems to be a general principle that you can't write to even XZR twice in the same instruction, so that ought to be more robust.

We should also add a test to test/CodeGen/AArch64.

Cheers.

Tim.

I'll try that. Just to make sure, will it distinguish between sp and zr (e.g. ignore a stack load)

Yep, they're completely different registers as far as LLVM is concerned.

Tim.

I'd like to get this resolved as well. I think it is related to PR 23209 (https://llvm.org/bugs/show_bug.cgi?id=23209).

yuyichao updated this revision to Diff 52465.Apr 2 2016, 9:39 AM
yuyichao edited edge metadata.

Add a check for existing zero register use. There's actually tests for this already but the tests were wrong...

Is there a way to check if the pass keeps ldxp xzr, x.., [...] unchanged if the second destination is dead?

Ping. Anything else I should change?

t.p.northover accepted this revision.Apr 8 2016, 11:41 AM
t.p.northover edited edge metadata.

A MIR test could handle the already-existing "ldp xzr, ..." case, but I don't think that's essential here. Thanks for the revisions!

Tim.

This revision is now accepted and ready to land.Apr 8 2016, 11:41 AM

So can this be merged?

I think so.

Tim.

Could you please commit it for me please, since I don't have commit access.

Thx.

t.p.northover closed this revision.Apr 13 2016, 9:31 AM

Yep, it should be committed as r266206.

Tim.