This is an archive of the discontinued LLVM Phabricator instance.

[LLVM][AArch64] Don't warn about clobbering X16 when Speculative Load Hardening is used
ClosedPublic

Authored by DavidSpickett on Sep 13 2022, 3:35 AM.

Details

Summary

SLH will fall back to a different technique if X16 is being used,
so there is no need to warn for inline asm use. Only prevent other codegen
from using it.

Diff Detail

Event Timeline

DavidSpickett created this revision.Sep 13 2022, 3:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2022, 3:35 AM
DavidSpickett requested review of this revision.Sep 13 2022, 3:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2022, 3:35 AM

newline at end of test

Add comment to clarify test purpose.
clang-format.

I seem to remember that the speculative_load_hardening for AArch64 has a work-around when a user does reserve register X16 in inline assembly, see comment at https://github.com/llvm/llvm-project/blob/b7dae832e61da1f8b48cce1715514cbd5809eb3f/llvm/lib/Target/AArch64/AArch64SpeculationHardening.cpp#L35.
Given that comment and the tests in https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/AArch64/speculation-hardening-dagisel.ll that check that behaviour, the warning/error implemented in this patch is not correct?

llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
342

I see that I've used the term "taint register" in the source code of AArch64SpeculationHardening.cpp; but I don't see the term in the (X86-centric) design document about Speculative Load Hardening at https://llvm.org/docs/SpeculativeLoadHardening.html. I don't immediately see an alternative term to clearly name the register that Speculative Load Hardening reserves.
Therefore, I think that using the term "taint register" is indeed the best option available here.

the warning/error implemented in this patch is not correct?

I see what you mean. This is a unfortunate crossover where AArch64RegisterInfo::getReservedRegs is not just used for inline assembly but codegen too. So if codegen wants X16 it can't use it but in the codegen for SLH you've actually handled the inline asm case so there's no reason to warn at all.

Running the test you linked we see that it does already warn when it doesn't need to:

--
Exit Code: 0

Command Output (stderr):
--
warning: inline asm clobber list contains reserved registers: W16
note: Reserved registers on the clobber list may not be preserved across the asm statement, and clobbering them may lead to undefined behaviour.
f_clobbered_reg_w16:                    // @f_clobbered_reg_w16
        .cfi_startproc
// %bb.0:                               // %entry
        dsb     sy
        isb
        cmp     w0, w1
        b.le    .LBB1_3
// %bb.1:
        dsb     sy
        isb
// %bb.2:                               // %if.then
        //APP
warning: inline asm clobber list contains reserved registers: W16
note: Reserved registers on the clobber list may not be preserved across the asm statement, and clobbering them may lead to undefined behaviour.
        mov     w16, w0
        //NO_APP
        add     w0, w1, w0
        ret

I think I can make the inline asm check more specific to avoid this but keep the register reserved for codegen.

llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
342

I could just go with "is used as part of Speculative Load Hardening". The point is, you can google something and land on an llvm page.

Instead, don't warn for inline asm clobber of x16 at all.

DavidSpickett retitled this revision from [LLVM][AArch64] Explain why X16 is reserved when Speculative Load Hardening is in use to [LLVM][AArch64] Don't warn about clobbering X16 when Speculative Load Hardening is used.Sep 13 2022, 8:20 AM
DavidSpickett edited the summary of this revision. (Show Details)
kristof.beyls accepted this revision.Sep 13 2022, 8:55 AM
kristof.beyls added inline comments.
llvm/test/CodeGen/AArch64/speculation-hardening-dagisel.ll
4–7 ↗(On Diff #459757)

The patch LGTM.
The only thing I was wondering whas if the test case could be hardened a bit further against the warning text changing in the future. If it does, the 2 CHECK-NOT lines probably would no longer be checking what is intended and no-one would notice.
I can't come up with a really good solution - so let's commit this as is.

This revision is now accepted and ready to land.Sep 13 2022, 8:55 AM
DavidSpickett added inline comments.Sep 14 2022, 7:51 AM
llvm/test/CodeGen/AArch64/speculation-hardening-dagisel.ll
4–7 ↗(On Diff #459757)

I might be able to reserve x16 using the equivalent of the command line option, for the non SLH run. Check that you do get the warning.

Then the test will fail if someone were to update it.

DavidSpickett marked an inline comment as done.Sep 14 2022, 8:09 AM
DavidSpickett added inline comments.
llvm/test/CodeGen/AArch64/speculation-hardening-dagisel.ll
4–7 ↗(On Diff #459757)

Turns out the way that option works, you can't use -ffixed-x16 because clang ignores it. It does this for any register deemed too useful aka used in the ABI.

(at least clang tells you the argument is unused, gcc is just silent)

I guess we hope that future warning editors are smart enough to grep the test folders for the old phrasing.

This revision was landed with ongoing or failed builds.Sep 14 2022, 8:19 AM
This revision was automatically updated to reflect the committed changes.