[AArch64] Support reserving x16 and x17 register

[phosek]

Summary

Registers x16 and 17 are defined as linker temporaries, which are used
for purposes such as veneers. However, in some cases these may be used
for other purposes in certain contexts, for example to hold special
variables within the kernel.

We would like to use these registers to hold the per-CPU pointer in Fuchsia
kernel. This change adds support for reserving these registers both to
frontend and backend to make these registers usable for such purpose.

[mcgrathr]

I don't know the backend or the register allocator at all but this looks like a thorough parallel of the existing model for -ffixed-x18.

llvm/lib/Target/AArch64/AArch64Subtarget.cpp

155

We could imagine dying default reserved-x16 or reserved-x17 to -mcmodel=kernel (as we do with TIPIDR_EL1 e.g.) but that's probably not worthwhile since we want the flexibility to change what we're using in the kernel.

llvm/test/CodeGen/AArch64/arm64-platform-reg.ll

24

Possibly worth separately testing combinations of reserve-{x16,x17,x18}. It would be a bug if max register pressure used any when more than one is enabled.

[kristof.beyls]

I have some doubts this is the right long term approach.

As https://android.googlesource.com/kernel/common/+/c0385b24af15020a1e505f2c984db0d7c0d017e1%5E%21/#F4 indicates, there are others who have use cases for reserving other registers than x16,x17,x18. (The linked patch reserves x1,x2,x3,x4,x5,x6,x7).
With that indication, I think it may be better to bite the bullet now and implement full support for gcc command line option "-ffixed-reg", where reg is, according to the gcc documentation: "reg must be the name of a register. The register names accepted are machine-specific and are defined in the REGISTER_NAMES macro in the machine description macro file.".
For the full documentation of this option, see https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html.

What do you think?

Thanks,

Kristof

[phosek]

In D46215#1081943, @kristof.beyls wrote:

I have some doubts this is the right long term approach.

As https://android.googlesource.com/kernel/common/+/c0385b24af15020a1e505f2c984db0d7c0d017e1%5E%21/#F4 indicates, there are others who have use cases for reserving other registers than x16,x17,x18. (The linked patch reserves x1,x2,x3,x4,x5,x6,x7).
With that indication, I think it may be better to bite the bullet now and implement full support for gcc command line option "-ffixed-reg", where reg is, according to the gcc documentation: "reg must be the name of a register. The register names accepted are machine-specific and are defined in the REGISTER_NAMES macro in the machine description macro file.".
For the full documentation of this option, see https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html.

What do you think?

Thanks,

Kristof

That was original proposal bug @chandlerc suggested in our offline discussion that it's not easily doable. It seems like we might be able to duplicate the same code for all remaining registers which will require a lot of duplicated logic but it should work? However, I might be missing some details of the register allocator which will break.

[kristof.beyls]

In D46215#1083481, @phosek wrote:

In D46215#1081943, @kristof.beyls wrote:

I have some doubts this is the right long term approach.

As https://android.googlesource.com/kernel/common/+/c0385b24af15020a1e505f2c984db0d7c0d017e1%5E%21/#F4 indicates, there are others who have use cases for reserving other registers than x16,x17,x18. (The linked patch reserves x1,x2,x3,x4,x5,x6,x7).
With that indication, I think it may be better to bite the bullet now and implement full support for gcc command line option "-ffixed-reg", where reg is, according to the gcc documentation: "reg must be the name of a register. The register names accepted are machine-specific and are defined in the REGISTER_NAMES macro in the machine description macro file.".
For the full documentation of this option, see https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html.

What do you think?

Thanks,

Kristof

That was original proposal bug @chandlerc suggested in our offline discussion that it's not easily doable. It seems like we might be able to duplicate the same code for all remaining registers which will require a lot of duplicated logic but it should work? However, I might be missing some details of the register allocator which will break.

My hope was that the most non-trivial part would be to replicate the gcc implementation detail described as "The register names accepted are machine-specific and are defined in the REGISTER_NAMES macro in the machine description macro file." in the documentation.

But let's see if @qcolombet or @MatzeB know any reason why there would be more complexities to support the generic gcc command line option.

[phosek]

We're going to use D46552 instead.

clang/docs/ClangCommandLineReference.rst

 .. option:: -mwindows<arg>
 
 .. option:: -mx32
 
 .. option:: -mxgot, -mno-xgot
 
 AARCH64
 -------
+.. option:: -ffixed-x16
+
+Reserve the x16 register (AArch64 only)
+
+.. option:: -ffixed-x17
+
+Reserve the x17 register (AArch64 only)
+
 .. option:: -ffixed-x18
 
 Reserve the x18 register (AArch64 only)
 
 .. option:: -mfix-cortex-a53-835769, -mno-fix-cortex-a53-835769
 
 Workaround Cortex-A53 erratum 835769 (AArch64 only)
 

clang/include/clang/Driver/Options.td

 def mgeneral_regs_only : Flag<["-"], "mgeneral-regs-only">, Group<m_aarch64_Features_Group>,
   HelpText<"Generate code which only uses the general purpose registers (AArch64 only)">;
 def mfix_cortex_a53_835769 : Flag<["-"], "mfix-cortex-a53-835769">,
   Group<m_aarch64_Features_Group>,
   HelpText<"Workaround Cortex-A53 erratum 835769 (AArch64 only)">;
 def mno_fix_cortex_a53_835769 : Flag<["-"], "mno-fix-cortex-a53-835769">,
   Group<m_aarch64_Features_Group>,
   HelpText<"Don't workaround Cortex-A53 erratum 835769 (AArch64 only)">;
+def ffixed_x16 : Flag<["-"], "ffixed-x16">, Group<m_aarch64_Features_Group>,
+  HelpText<"Reserve the x16 register (AArch64 only)">;
+def ffixed_x17 : Flag<["-"], "ffixed-x17">, Group<m_aarch64_Features_Group>,
+  HelpText<"Reserve the x17 register (AArch64 only)">;
 def ffixed_x18 : Flag<["-"], "ffixed-x18">, Group<m_aarch64_Features_Group>,
   HelpText<"Reserve the x18 register (AArch64 only)">;
 
 def msimd128 : Flag<["-"], "msimd128">, Group<m_wasm_Features_Group>;
 def mno_simd128 : Flag<["-"], "mno-simd128">, Group<m_wasm_Features_Group>;
 def mnontrapping_fptoint : Flag<["-"], "mnontrapping-fptoint">, Group<m_wasm_Features_Group>;
 def mno_nontrapping_fptoint : Flag<["-"], "mno-nontrapping-fptoint">, Group<m_wasm_Features_Group>;
 def msign_ext : Flag<["-"], "msign-ext">, Group<m_wasm_Features_Group>;

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

else
       Features.push_back("-crc");
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_mno_unaligned_access,
                                options::OPT_munaligned_access))
     if (A->getOption().matches(options::OPT_mno_unaligned_access))
       Features.push_back("+strict-align");
 
+  if (Args.hasArg(options::OPT_ffixed_x16))
+    Features.push_back("+reserve-x16");
+
+  if (Args.hasArg(options::OPT_ffixed_x17))
+    Features.push_back("+reserve-x17");
+
   if (Args.hasArg(options::OPT_ffixed_x18))
     Features.push_back("+reserve-x18");
 
   if (Args.hasArg(options::OPT_mno_neg_immediates))
     Features.push_back("+no-neg-immediates");
 }

clang/test/Driver/aarch64-fixed-x16.c

+// RUN: %clang -target aarch64-none-gnu -ffixed-x16 -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-FIXED-X16 < %t %s
+
+// CHECK-FIXED-X16: "-target-feature" "+reserve-x16"

clang/test/Driver/aarch64-fixed-x17.c

+// RUN: %clang -target aarch64-none-gnu -ffixed-x17 -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-FIXED-X17 < %t %s
+
+// CHECK-FIXED-X17: "-target-feature" "+reserve-x17"

llvm/lib/Target/AArch64/AArch64.td

def FeatureZCZeroingFPWorkaround : SubtargetFeature<"zcz-fp-workaround",
     "HasZeroCycleZeroingFPWorkaround", "true",
     "The zero-cycle floating-point zeroing instruction has a bug">;
 
 def FeatureStrictAlign : SubtargetFeature<"strict-align",
                                           "StrictAlign", "true",
                                           "Disallow all unaligned memory "
                                           "access">;
 
+def FeatureReserveX16 : SubtargetFeature<"reserve-x16", "ReserveX16", "true",
+                                         "Reserve X16, making it unavailable "
+                                         "as a GPR">;
+
+def FeatureReserveX17 : SubtargetFeature<"reserve-x17", "ReserveX17", "true",
+                                         "Reserve X17, making it unavailable "
+                                         "as a GPR">;
+
 def FeatureReserveX18 : SubtargetFeature<"reserve-x18", "ReserveX18", "true",
                                          "Reserve X18, making it unavailable "
                                          "as a GPR">;
 
 def FeatureUseAA : SubtargetFeature<"use-aa", "UseAA", "true",
                                     "Use alias analysis during codegen">;
 
 def FeatureBalanceFPOps : SubtargetFeature<"balance-fp-ops", "BalanceFPOps",

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

 }
 
 // FIXME? Maybe this could be a TableGen attribute on some registers and
 // this table could be generated automatically from RegInfo.
 unsigned AArch64TargetLowering::getRegisterByName(const char* RegName, EVT VT,
                                                   SelectionDAG &DAG) const {
   unsigned Reg = StringSwitch<unsigned>(RegName)
                        .Case("sp", AArch64::SP)
+                       .Case("x16", AArch64::X16)
+                       .Case("w16", AArch64::W16)
+                       .Case("x17", AArch64::X17)
+                       .Case("w17", AArch64::W17)
                        .Case("x18", AArch64::X18)
                        .Case("w18", AArch64::W18)
                        .Default(0);
-  if ((Reg == AArch64::X18 || Reg == AArch64::W18) &&
-      !Subtarget->isX18Reserved())
+  if (((Reg == AArch64::X16 || Reg == AArch64::W16) &&
+      !Subtarget->isX16Reserved()) ||
+      ((Reg == AArch64::X17 || Reg == AArch64::W17) &&
+      !Subtarget->isX17Reserved()) ||
+      ((Reg == AArch64::X18 || Reg == AArch64::W18) &&
+      !Subtarget->isX18Reserved()))
     Reg = 0;
   if (Reg)
     return Reg;
   report_fatal_error(Twine("Invalid register name \""
                               + StringRef(RegName)  + "\"."));
 }
 
 SDValue AArch64TargetLowering::LowerRETURNADDR(SDValue Op,

llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp

AArch64RegisterInfo::getReservedRegs(const MachineFunction &MF) const {
   // FIXME: avoid re-calculating this every time.
   BitVector Reserved(getNumRegs());
   markSuperRegs(Reserved, AArch64::WSP);
   markSuperRegs(Reserved, AArch64::WZR);
 
   if (TFI->hasFP(MF) || TT.isOSDarwin())
     markSuperRegs(Reserved, AArch64::W29);
 
+  if (MF.getSubtarget<AArch64Subtarget>().isX16Reserved())
+    markSuperRegs(Reserved, AArch64::W16); // Platform register
+
+  if (MF.getSubtarget<AArch64Subtarget>().isX17Reserved())
+    markSuperRegs(Reserved, AArch64::W17); // Platform register
+
   if (MF.getSubtarget<AArch64Subtarget>().isX18Reserved())
     markSuperRegs(Reserved, AArch64::W18); // Platform register
 
   if (hasBasePointer(MF))
     markSuperRegs(Reserved, AArch64::W19);
 
   assert(checkAllSuperRegsMarked(Reserved));
   return Reserved;
 }
 
 bool AArch64RegisterInfo::isReservedReg(const MachineFunction &MF,
                                       unsigned Reg) const {
   const AArch64FrameLowering *TFI = getFrameLowering(MF);
 
   switch (Reg) {
   default:
     break;
   case AArch64::SP:
   case AArch64::XZR:
   case AArch64::WSP:
   case AArch64::WZR:
     return true;
+  case AArch64::X16:
+  case AArch64::W16:
+    return MF.getSubtarget<AArch64Subtarget>().isX16Reserved();
+  case AArch64::X17:
+  case AArch64::W17:
+    return MF.getSubtarget<AArch64Subtarget>().isX17Reserved();
   case AArch64::X18:
   case AArch64::W18:
     return MF.getSubtarget<AArch64Subtarget>().isX18Reserved();
   case AArch64::FP:
   case AArch64::W29:
     return TFI->hasFP(MF) || TT.isOSDarwin();
   case AArch64::W19:
   case AArch64::X19:
unsigned AArch64RegisterInfo::getRegPressureLimit(const TargetRegisterClass *RC,
   case AArch64::GPR64spRegClassID:
   case AArch64::GPR64allRegClassID:
   case AArch64::GPR64RegClassID:
   case AArch64::GPR32commonRegClassID:
   case AArch64::GPR64commonRegClassID:
     return 32 - 1                                   // XZR/SP
               - (TFI->hasFP(MF) || TT.isOSDarwin()) // FP
               - MF.getSubtarget<AArch64Subtarget>()
+                    .isX16Reserved() // X16 reserved as platform register
+              - MF.getSubtarget<AArch64Subtarget>()
+                    .isX17Reserved() // X17 reserved as platform register
+              - MF.getSubtarget<AArch64Subtarget>()
                     .isX18Reserved() // X18 reserved as platform register
               - hasBasePointer(MF);  // X19
   case AArch64::FPR8RegClassID:
   case AArch64::FPR16RegClassID:
   case AArch64::FPR32RegClassID:
   case AArch64::FPR64RegClassID:
   case AArch64::FPR128RegClassID:
     return 32;

llvm/lib/Target/AArch64/AArch64Subtarget.h

protected:
   uint16_t PrefetchDistance = 0;
   uint16_t MinPrefetchStride = 1;
   unsigned MaxPrefetchIterationsAhead = UINT_MAX;
   unsigned PrefFunctionAlignment = 0;
   unsigned PrefLoopAlignment = 0;
   unsigned MaxJumpTableSize = 0;
   unsigned WideningBaseCost = 0;
 
+  // ReserveX16 - X16 is not available as a general purpose register.
+  bool ReserveX16 = false;
+
+  // ReserveX17 - X17 is not available as a general purpose register.
+  bool ReserveX17 = false;
+
   // ReserveX18 - X18 is not available as a general purpose register.
   bool ReserveX18;
 
   bool IsLittle;
 
   /// TargetTriple - What processor and OS we're targeting.
   Triple TargetTriple;
 
public:
   bool requiresStrictAlign() const { return StrictAlign; }
 
   bool isXRaySupported() const override { return true; }
 
   unsigned getMinVectorRegisterBitWidth() const {
     return MinVectorRegisterBitWidth;
   }
 
+  bool isX16Reserved() const { return ReserveX16; }
+  bool isX17Reserved() const { return ReserveX17; }
   bool isX18Reserved() const { return ReserveX18; }
   bool hasFPARMv8() const { return HasFPARMv8; }
   bool hasNEON() const { return HasNEON; }
   bool hasCrypto() const { return HasCrypto; }
   bool hasDotProd() const { return HasDotProd; }
   bool hasCRC() const { return HasCRC; }
   bool hasLSE() const { return HasLSE; }
   bool hasRAS() const { return HasRAS; }

llvm/lib/Target/AArch64/AArch64Subtarget.cpp

void AArch64Subtarget::initializeProperties() {
   case Others: break;
   }
 }
 
 AArch64Subtarget::AArch64Subtarget(const Triple &TT, const std::string &CPU,
                                    const std::string &FS,
                                    const TargetMachine &TM, bool LittleEndian)
     : AArch64GenSubtargetInfo(TT, CPU, FS),
-      ReserveX18(AArch64::isX18ReservedByDefault(TT)), IsLittle(LittleEndian),
-      TargetTriple(TT), FrameLowering(),
+      ReserveX18(AArch64::isX18ReservedByDefault(TT)),
+      IsLittle(LittleEndian), TargetTriple(TT), FrameLowering(),
       InstrInfo(initializeSubtargetDependencies(FS, CPU)), TSInfo(),
       TLInfo(TM, *this) {
   CallLoweringInfo.reset(new AArch64CallLowering(*getTargetLowering()));
   Legalizer.reset(new AArch64LegalizerInfo(*this));
 
   auto *RBI = new AArch64RegisterBankInfo(*getRegisterInfo());
 
   // FIXME: At this point, we can't rely on Subtarget having RBI.

llvm/test/CodeGen/AArch64/arm64-platform-reg.ll

+; RUN: llc -mtriple=aarch64-fuchsia -mattr=+reserve-x16 -o - %s | FileCheck %s --check-prefix=CHECK-RESERVE-X16
+; RUN: llc -mtriple=aarch64-fuchsia -mattr=+reserve-x17 -o - %s | FileCheck %s --check-prefix=CHECK-RESERVE-X17
 ; RUN: llc -mtriple=arm64-apple-ios -mattr=+reserve-x18 -o - %s | FileCheck %s --check-prefix=CHECK-RESERVE-X18
 ; RUN: llc -mtriple=arm64-freebsd-gnu -mattr=+reserve-x18 -o - %s | FileCheck %s --check-prefix=CHECK-RESERVE-X18
 ; RUN: llc -mtriple=arm64-linux-gnu -o - %s | FileCheck %s
 ; RUN: llc -mtriple=aarch64-fuchsia -o - %s | FileCheck %s --check-prefix=CHECK-RESERVE-X18
 ; RUN: llc -mtriple=aarch64-windows -o - %s | FileCheck %s --check-prefix=CHECK-RESERVE-X18
 
 ; x18 is reserved as a platform register on Darwin but not on other
 ; systems. Create loads of register pressure and make sure this is respected.
 
 ; Also, fp must always refer to a valid frame record, even if it's not the one
 ; of the current function, so it shouldn't be used either.
 
 @var = global [30 x i64] zeroinitializer
 
 define void @keep_live() {
   %val = load volatile [30 x i64], [30 x i64]* @var
   store volatile [30 x i64] %val, [30 x i64]* @var
 
 ; CHECK: ldr x18
 ; CHECK: str x18
 
+; CHECK-RESERVE-X16-NOT: ldr fp
+; CHECK-RESERVE-X16-NOT: ldr x16
+; CHECK-RESERVE-X16: Spill
+; CHECK-RESERVE-X16-NOT: ldr fp
+; CHECK-RESERVE-X16-NOT: ldr x16
+; CHECK-RESERVE-X16: ret
+
+; CHECK-RESERVE-X17-NOT: ldr fp
+; CHECK-RESERVE-X17-NOT: ldr x17
+; CHECK-RESERVE-X17: Spill
+; CHECK-RESERVE-X17-NOT: ldr fp
+; CHECK-RESERVE-X17-NOT: ldr x17
+; CHECK-RESERVE-X17: ret
+
 ; CHECK-RESERVE-X18-NOT: ldr fp
 ; CHECK-RESERVE-X18-NOT: ldr x18
 ; CHECK-RESERVE-X18: Spill
 ; CHECK-RESERVE-X18-NOT: ldr fp
 ; CHECK-RESERVE-X18-NOT: ldr x18
 ; CHECK-RESERVE-X18: ret
   ret void
 }