This is an archive of the discontinued LLVM Phabricator instance.

[Aarch64] Correct register class for pseudo instructions
ClosedPublic

Authored by loladiro on Feb 24 2021, 8:06 PM.

Details

Summary

This constrains the Mov* and similar pseudo instruction to take
GPR64common register classes rather than GPR64. GPR64 includs XZR
which is invalid here, because this pseudo instructions expands
into an adrp/add pair sharing a destination register. XZR is invalid
on add and attempting to encode it will instead increment the stack
pointer causing crashes (downstream report at [1]). The test case
there reproduces on LLVM11, but I do not have a test case that
reaches this code path on main, since it is being masked by
improved dead code elimination introduced in D91513. Nevertheless,
this seems like a good thing to fix in case there are other cases
that dead code elimination doesn't clean up (e.g. if optnone is
used and the optimization is skipped).

I think it would be worth auditing uses of GPR64 in pseudo
instructions to see if there are any similar issues, but I do not
have a high enough view of the backend or knowledge of the
Aarch64 architecture to do this quickly.

[1] https://github.com/JuliaLang/julia/issues/39818

Diff Detail

Event Timeline

loladiro created this revision.Feb 24 2021, 8:06 PM
loladiro requested review of this revision.Feb 24 2021, 8:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2021, 8:06 PM
t.p.northover accepted this revision.Feb 26 2021, 3:11 AM

Looks reasonable to me. If you were really keen you could probably write a MIR test (it complains loudly when an instruction has the wrong regclass), but in reality I think it's unlikely to matter. This isn't exactly a high-traffic part of the backend.

This revision is now accepted and ready to land.Feb 26 2021, 3:11 AM
loladiro updated this revision to Diff 327263.Mar 1 2021, 1:23 PM

Updated to add updates to test cases I'd forgotten to include in the revision.
They're entirely mechanical, but I'd appreciate a quick sanity check.

t.p.northover accepted this revision.Mar 4 2021, 11:39 AM

Thanks, looks good to me.

vchuravy added a project: Restricted Project.Aug 28 2021, 8:41 AM
vchuravy added a subscriber: vchuravy.
vtjnash updated this revision to Diff 370017.Sep 1 2021, 12:06 PM
vtjnash added a subscriber: vtjnash.

rebase onto master

vtjnash updated this revision to Diff 370023.Sep 1 2021, 12:18 PM

fix another missed test update

This revision was landed with ongoing or failed builds.Sep 9 2021, 11:33 AM
This revision was automatically updated to reflect the committed changes.