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

[DavidSpickett]

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.

[DavidSpickett]

newline at end of test

[DavidSpickett]

Add comment to clarify test purpose.
clang-format.

[kristof.beyls]

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.

[DavidSpickett]

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.

[DavidSpickett]

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

[kristof.beyls]

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.

[DavidSpickett]

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]

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.

llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp

 //===- AArch64RegisterInfo.cpp - AArch64 Register Information -------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //

 
 const uint32_t *AArch64RegisterInfo::getWindowsStackProbePreservedMask() const {
   return CSR_AArch64_StackProbe_Windows_RegMask;
 }
 
 llvm::Optional<std::string>
 AArch64RegisterInfo::explainReservedReg(const MachineFunction &MF,
                                         MCRegister PhysReg) const {
+  // In cases like this where the clobber might be a W register, the message
+  // always uses the X name because the purpose it is reserved for uses the full
+  // 64 bits.
   if (hasBasePointer(MF) && MCRegisterInfo::regsOverlap(PhysReg, AArch64::X19))
     return std::string("X19 is used as the frame base pointer register.");
 
   if (MF.getSubtarget<AArch64Subtarget>().isWindowsArm64EC()) {
     bool warn = false;
     if (MCRegisterInfo::regsOverlap(PhysReg, AArch64::X13) ||
         MCRegisterInfo::regsOverlap(PhysReg, AArch64::X14) ||
         MCRegisterInfo::regsOverlap(PhysReg, AArch64::X23) ||
         MCRegisterInfo::regsOverlap(PhysReg, AArch64::X24) ||
         MCRegisterInfo::regsOverlap(PhysReg, AArch64::X28))
       warn = true;
 
     for (unsigned i = AArch64::B16; i <= AArch64::B31; ++i)
       if (MCRegisterInfo::regsOverlap(PhysReg, i))
         warn = true;
 
     if (warn)
       return std::string(AArch64InstPrinter::getRegisterName(PhysReg)) +
              " is clobbered by asynchronous signals when using Arm64EC.";
   }
 
+  if (MF.getFunction().hasFnAttribute(Attribute::SpeculativeLoadHardening) &&
+      MCRegisterInfo::regsOverlap(PhysReg, AArch64::X16))
+    return std::string(
+        "X16 is used as the taint register for Speculative Load Hardening.");
+
   return {};
 }
 
 BitVector
 AArch64RegisterInfo::getStrictlyReservedRegs(const MachineFunction &MF) const {
   const AArch64FrameLowering *TFI = getFrameLowering(MF);
 
   // FIXME: avoid re-calculating this every time.

llvm/test/CodeGen/AArch64/inline-asm-clobber-slh-taint.ll

+; RUN: llc -mtriple=aarch64-none-linux-gnu %s 2>&1 | FileCheck %s
+
+; Check that not only do we warn about X16 being reserved, we also print an explanation note.
+
+; CHECK: warning: inline asm clobber list contains reserved registers: X16
+; CHECK-NEXT: note: Reserved registers on the clobber list may not be preserved across the asm statement, and clobbering them may lead to undefined behaviour.
+; CHECK-NEXT: note: X16 is used as the taint register for Speculative Load Hardening.
+
+define void @fx() speculative_load_hardening {
+entry:
+  call void asm sideeffect "nop", "~{x16}"()
+  ret void
+}
+
+; CHECK: warning: inline asm clobber list contains reserved registers: W16
+; CHECK-NEXT: note: Reserved registers on the clobber list may not be preserved across the asm statement, and clobbering them may lead to undefined behaviour.
+; CHECK-NEXT: note: X16 is used as the taint register for Speculative Load Hardening.
+
+define void @fw() speculative_load_hardening {
+entry:
+  call void asm sideeffect "nop", "~{w16}"()
+  ret void
+}