[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

339

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

339

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

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

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

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

if (MF.getSubtarget<AArch64Subtarget>().isWindowsArm64EC()) {
     if (warn)
       return std::string(AArch64InstPrinter::getRegisterName(PhysReg)) +
              " is clobbered by asynchronous signals when using Arm64EC.";
   }
 
   return {};
 }
 
 BitVector
 AArch64RegisterInfo::getStrictlyReservedRegs(const MachineFunction &MF) const {
   const AArch64FrameLowering *TFI = getFrameLowering(MF);
 
   // FIXME: avoid re-calculating this every time.
   BitVector Reserved(getNumRegs());
   markSuperRegs(Reserved, AArch64::WSP);
   markSuperRegs(Reserved, AArch64::WZR);
 
void AArch64RegisterInfo::emitReservedArgRegCallError(
     const MachineFunction &MF) const {
   const Function &F = MF.getFunction();
   F.getContext().diagnose(DiagnosticInfoUnsupported{F, ("AArch64 doesn't support"
     " function calls if any of the argument registers is reserved.")});
 }
 
 bool AArch64RegisterInfo::isAsmClobberable(const MachineFunction &MF,
                                           MCRegister PhysReg) const {
+  // SLH uses register X16 as the taint register but it will fallback to a different
+  // method if the user clobbers it. So X16 is not reserved for inline asm but is
+  // for normal codegen.
+  if (MF.getFunction().hasFnAttribute(Attribute::SpeculativeLoadHardening) &&
+        MCRegisterInfo::regsOverlap(PhysReg, AArch64::X16))
+    return true;
+
   return !isReservedReg(MF, PhysReg);
 }
 
 const TargetRegisterClass *
 AArch64RegisterInfo::getPointerRegClass(const MachineFunction &MF,
                                       unsigned Kind) const {
   return &AArch64::GPR64spRegClass;
 }

llvm/test/CodeGen/AArch64/speculation-hardening-dagisel.ll

-; RUN: sed -e 's/SLHATTR/speculative_load_hardening/' %s | llc -verify-machineinstrs -mtriple=aarch64-none-linux-gnu | FileCheck %s --check-prefixes=CHECK,SLH
-; RUN: sed -e 's/SLHATTR//' %s | llc -verify-machineinstrs -mtriple=aarch64-none-linux-gnu | FileCheck %s --check-prefixes=CHECK,NOSLH
+; RUN: sed -e 's/SLHATTR/speculative_load_hardening/' %s | llc -verify-machineinstrs -mtriple=aarch64-none-linux-gnu 2>&1 | FileCheck %s --check-prefixes=CHECK,SLH
+; RUN: sed -e 's/SLHATTR//' %s | llc -verify-machineinstrs -mtriple=aarch64-none-linux-gnu 2>&1 | FileCheck %s --check-prefixes=CHECK,NOSLH
+
+; As SLH is falling back to a technique that doesn't use X16, we shouldn't see any warnings about clobbers.
+; (this would come from f_clobbered_reg_w16, but warnings are first in the output)
+; CHECK-NOT: warning: inline asm clobber list contains reserved registers: W16
+; CHECK-NOT: warning: inline asm clobber list contains reserved registers: X16
 
 declare i64 @g(i64, i64) local_unnamed_addr
 define i64 @f_using_reserved_reg_x16(i64 %a, i64 %b) local_unnamed_addr SLHATTR {
 ; CHECK-LABEL: f_using_reserved_reg_x16
 ; SLH: dsb sy
 ; SLH: isb
 ; NOSLH-NOT: dsb sy
 ; NOSLH-NOT: isb