[AArch64] Treat x18 as callee-saved in functions with windows calling convention on non-windows OSes

[mstorsjo]

Summary

Treat it as callee-saved, and always back it up. When windows code calls entry points in unix code, marked with the windows calling convention, that unix code can call other functions that isn't compiled with -ffixed-x18 which may clobber x18 freely. By backing it up and restoring it on return, we preserve the register across the function call, fulfilling this part of the windows calling convention on another OS.

This isn't enough for making sure that x18 is preseved when non-windows code does a callback to windows code, but is a clear improvement over the current status quo. Additionally, wine is nowadays building many modules as PE DLLs, which avoids the callback issue altogether for those DLLs.

[mstorsjo]

Removing the RFC marking from this one, with the intention of getting it merged. It's a pretty non-intrusive (and cheap) fix and for maintaining x18 across wine built in functions - I've tested it in daily use for a month now and the fix does seem to behave as intended. Wine builds most DLLs as PE DLLs these days, so callbacks is no longer an issue for the built-in modules built as PE DLLs.

[mstorsjo]

This comment was removed by mstorsjo.

[mstorsjo]

@rnk - if you have time before you go on your leave - could you have a look at this one?

[mstorsjo]

Ping - who can review AArch64 backend stuff?

[efriedma]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2447 ↗

(On Diff #264837)

This change shouldn't be necessary. If some code in the function modifies x18, or calls some non-Win64 function, x18 will be clobbered. Frame lowering should see that, and figure out it needs to save x18 without any extra help.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

3947 ↗

(On Diff #264837)

Do we want to also check that the callee CC isn't Win64?

[mstorsjo]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2447 ↗

(On Diff #264837)

No, this is actually the core of the intent of this change.

When windows code (which expects x18 to be unmodified) calls an entry point in wine (marked with the windows calling convention), the entry point function (linux code but with a windows calling convention) itself probably won't clobber x18 (it's potentially even compiled with -ffixed-x18 to avoid doing that) - but this function can call other functions (glibc, other userland libraries), and these functions can freely clobber x18.

So this is meant to always restore x18 on return from a windows calling convention function, as we can't really know what happened meanwhile.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

3947 ↗

(On Diff #264837)

Hmm, indeed - in that case we could actually allow the tail call, yes. I'll see if I can add a test for that case.

[mstorsjo]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

3947 ↗

(On Diff #264837)

Actually, we don't produce tail calls for the Win64 calling convention at all right now - it isn't in mayTailCallThisCC above at all. I guess that _should_ be fine but I'm not sure if it'd break anything, and I think adding it in this patch would be a bit out of scope. I can add that bit into the condition here though, but that case wouldn't be usable just yet at least, and not be tested here.

[mstorsjo]

Clarified the comment about always backing up x18, added a condition about CalleeCC.

[efriedma]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2447 ↗

(On Diff #264837)

Hmm...

So the functions using the Windows calling convention are compiled with -ffixed-x18, which means they don't treat x18 as allocatable at all. And potentially, Unix calling convention code in the same file is compiled the same way. But eventually, somewhere in the callstack, there might be a function that clobbers x18, so you want to save/restore it despite the fact that it's marked "fixed".

If x18 is fixed anyway, why do you need to mess with AArch64RegisterInfo::getCalleeSavedRegs? It shouldn't participate in any computation related to the calling convention.

It seems a little magical to me that functions using the Win64 calling convention are making assumptions about the behavior of x18 despite the -ffixed-x18 marking. The alternative would be to add a function attribute to specifically request this behavior, I guess, separate from the calling convention. That would separate the concerns a bit more, but realistically I'm not sure anyone other than Wine is using the Win64 convention on non-Windows targets, so maybe it's not important.

[mstorsjo]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2447 ↗

(On Diff #264837)

Well, technically, the functions don't need to be compiled with -ffixed-x18, strictly, this should patch works the same either way, just that adding that flag reduces the risk of it being clobbered while in the wine code itself as well. (Fixing that aspect properly is a different issue which is handled in a different way.)

If x18 is fixed anyway, why do you need to mess with AArch64RegisterInfo::getCalleeSavedRegs? It shouldn't participate in any computation related to the calling convention.

If x18 is fixed, we don't need that bit, but if it isn't, it's necessary.

realistically I'm not sure anyone other than Wine is using the Win64 convention on non-Windows targets, so maybe it's not important.

Yeah I don't think practically any other code base really would be using it (other than potential windows binary loaders used in other projects, i.e. doing the same - but that's probably not really prevalent for anything else than x86).

[efriedma]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2447 ↗

(On Diff #264837)

Please make sure we have test coverage for both the fixed-x18 and non-fixed-x18 cases, then.

[mstorsjo]

Added a testcase with -mattr=+reserve-x18, in addition to the previous one without it.

[efriedma]

LGTM

lib/Target/AArch64/AArch64CallingConvention.td

 // It would be better to model its preservation semantics properly (create a
 // vreg on entry, use it in RET & tail call generation; make that vreg def if we
 // end up saving LR as part of a call frame). Watch this space...
 def CSR_AArch64_AAPCS : CalleeSavedRegs<(add LR, FP, X19, X20, X21, X22,
                                            X23, X24, X25, X26, X27, X28,
                                            D8,  D9,  D10, D11,
                                            D12, D13, D14, D15)>;
 
+// A variant for treating X18 as callee saved, when interfacing with
+// code that needs X18 to be preserved.
+def CSR_AArch64_AAPCS_X18 : CalleeSavedRegs<(add CSR_AArch64_AAPCS, X18)>;
+
 // Win64 has unwinding codes for an (FP,LR) pair, save_fplr and save_fplr_x.
 // We put FP before LR, so that frame lowering logic generates (FP,LR) pairs,
 // and not (LR,FP) pairs.
 def CSR_Win_AArch64_AAPCS : CalleeSavedRegs<(add FP, LR, X19, X20, X21, X22,
                                                X23, X24, X25, X26, X27, X28,
                                                D8, D9, D10, D11,
                                                D12, D13, D14, D15)>;
 

lib/Target/AArch64/AArch64FrameLowering.cpp

   // All calls are tail calls in GHC calling conv, and functions have no
   // prologue/epilogue.
   if (MF.getFunction().getCallingConv() == CallingConv::GHC)
     return;
 
   TargetFrameLowering::determineCalleeSaves(MF, SavedRegs, RS);
   const AArch64RegisterInfo *RegInfo = static_cast<const AArch64RegisterInfo *>(
       MF.getSubtarget().getRegisterInfo());
+  const AArch64Subtarget &Subtarget = MF.getSubtarget<AArch64Subtarget>();
   AArch64FunctionInfo *AFI = MF.getInfo<AArch64FunctionInfo>();
   unsigned UnspilledCSGPR = AArch64::NoRegister;
   unsigned UnspilledCSGPRPaired = AArch64::NoRegister;
 
   MachineFrameInfo &MFI = MF.getFrameInfo();
   const MCPhysReg *CSRegs = MF.getRegInfo().getCalleeSavedRegs();
 
   unsigned BasePointerReg = RegInfo->hasBasePointer(MF)

   // The frame record needs to be created by saving the appropriate registers
   unsigned EstimatedStackSize = MFI.estimateStackSize(MF);
   if (hasFP(MF) ||
       windowsRequiresStackProbe(MF, EstimatedStackSize + CSStackSize + 16)) {
     SavedRegs.set(AArch64::FP);
     SavedRegs.set(AArch64::LR);
   }
 
+  if (MF.getFunction().getCallingConv() == CallingConv::Win64 &&
+      !Subtarget.isTargetWindows() && Subtarget.isXRegisterReserved(18)) {
+    // For Windows calling convention on a non-windows OS, where x18 is treated
+    // as reserved, back up X18 when entering non-windows code (marked with the
+    // Windows calling convention) and restore when returning.
+    SavedRegs.set(AArch64::X18);
+  }
+
   LLVM_DEBUG(dbgs() << "*** determineCalleeSaves\nUsed CSRs:";
              for (unsigned Reg
                   : SavedRegs.set_bits()) dbgs()
              << ' ' << printReg(Reg, RegInfo);
              dbgs() << "\n";);
 
   // If any callee-saved registers are used, the frame cannot be eliminated.
   bool CanEliminateFrame = SavedRegs.count() == 0;

lib/Target/AArch64/AArch64ISelLowering.cpp

   if (!mayTailCallThisCC(CalleeCC))
     return false;
 
   MachineFunction &MF = DAG.getMachineFunction();
   const Function &CallerF = MF.getFunction();
   CallingConv::ID CallerCC = CallerF.getCallingConv();
   bool CCMatch = CallerCC == CalleeCC;
 
+  // When using the Windows calling convention on a non-windows OS, we want
+  // to back up and restore X18 in such functions; we can't do a tail call
+  // from those functions.
+  if (CallerCC == CallingConv::Win64 && !Subtarget->isTargetWindows() &&
+      Subtarget->isXRegisterReserved(18))
+    return false;
+
   // Byval parameters hand the function a pointer directly into the stack area
   // we want to reuse during a tail call. Working around this *is* possible (see
   // X86) but less efficient and uglier in LowerCall.
   for (Function::const_arg_iterator i = CallerF.arg_begin(),
                                     e = CallerF.arg_end();
        i != e; ++i) {
     if (i->hasByValAttr())
       return false;

lib/Target/AArch64/AArch64RegisterInfo.cpp

return MF->getInfo<AArch64FunctionInfo>()->isSplitCSR() ?
            CSR_AArch64_CXX_TLS_Darwin_SaveList;
   if (MF->getSubtarget<AArch64Subtarget>().getTargetLowering()
           ->supportSwiftError() &&
       MF->getFunction().getAttributes().hasAttrSomewhere(
           Attribute::SwiftError))
     return CSR_AArch64_AAPCS_SwiftError_SaveList;
   if (MF->getFunction().getCallingConv() == CallingConv::PreserveMost)
     return CSR_AArch64_RT_MostRegs_SaveList;
+  else if (MF->getFunction().getCallingConv() == CallingConv::Win64 &&
+           MF->getSubtarget<AArch64Subtarget>().isXRegisterReserved(18))
+    // This is for OSes other than Windows; Windows is a separate case further
+    // above.
+    return CSR_AArch64_AAPCS_X18_SaveList;
   else
     return CSR_AArch64_AAPCS_SaveList;
 }
 
 const MCPhysReg *AArch64RegisterInfo::getCalleeSavedRegsViaCopy(
     const MachineFunction *MF) const {
   assert(MF && "Invalid MachineFunction pointer.");
   if (MF->getFunction().getCallingConv() == CallingConv::CXX_FAST_TLS &&

test/CodeGen/AArch64/win64cc-backup-x18.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=aarch64-linux-gnu | FileCheck %s
+
+declare dso_local void @other()
+
+define dso_local win64cc void @func() #0 {
+; CHECK-LABEL: func:
+; CHECK:       // %bb.0: // %entry
+; CHECK-NEXT:    stp x18, x30, [sp, #-16]! // 16-byte Folded Spill
+; CHECK-NEXT:    bl other
+; CHECK-NEXT:    ldp x18, x30, [sp], #16 // 16-byte Folded Reload
+; CHECK-NEXT:    ret
+entry:
+  tail call void @other()
+  ret void
+}
+
+attributes #0 = { nounwind "target-features"="+reserve-x18" }