Page MenuHomePhabricator

[Aarch64] Correct register class for pseudo instructions

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



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.


Diff Detail

Unit TestsFailed

300 msx64 debian > LLVM.CodeGen/AArch64::elim-dead-mi.mir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -mtriple=aarch64-arm-none-eabi -o - /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/elim-dead-mi.mir -run-pass dead-mi-elimination | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/elim-dead-mi.mir
310 msx64 debian > LLVM.CodeGen/AArch64::loop-sink.mir
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -mtriple aarch64 -run-pass=machine-sink -sink-insts-to-avoid-spills /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/loop-sink.mir -o - 2>&1 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/loop-sink.mir
50 msx64 debian > LLVM.CodeGen/AArch64/GlobalISel::select-blockaddress.mir
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -mtriple=aarch64-unknown-unknown -o - -verify-machineinstrs -run-pass=instruction-select /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/select-blockaddress.mir | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/select-blockaddress.mir
40 msx64 debian > LLVM.CodeGen/AArch64/GlobalISel::select-jump-table-brjt-constrain.mir
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -mtriple=aarch64-unknown-unknown -o - -verify-machineinstrs -run-pass=instruction-select /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/select-jump-table-brjt-constrain.mir | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/select-jump-table-brjt-constrain.mir
30 msx64 debian > LLVM.CodeGen/AArch64/GlobalISel::select-jump-table-brjt.mir
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -mtriple=aarch64-unknown-unknown -o - -verify-machineinstrs -run-pass=instruction-select /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/select-jump-table-brjt.mir | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/select-jump-table-brjt.mir
View Full Test Results (14 Failed)

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.