Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AArch64] Disable variadic tail-calls for ARM64EC #78914

Closed
wants to merge 1 commit into from

Conversation

bylaws
Copy link
Contributor

@bylaws bylaws commented Jan 21, 2024

Arm64EC varargs calls expect that x4 = sp at entry, this complicates tail call handling so disable for now.

CC: @cjacek

Arm64EC varargs calls expect that x4 = sp at entry, this complicates
tail call handling so disable for now.
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 21, 2024

@llvm/pr-subscribers-backend-aarch64

Author: Billy Laws (bylaws)

Changes

Arm64EC varargs calls expect that x4 = sp at entry, this complicates tail call handling so disable for now.

CC: @cjacek


Full diff: https://github.com/llvm/llvm-project/pull/78914.diff

2 Files Affected:

  • (modified) llvm/lib/Target/AArch64/AArch64ISelLowering.cpp (+5)
  • (modified) llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp (+6)
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index 96ea692d03f563..264c301d81194f 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -7235,6 +7235,11 @@ bool AArch64TargetLowering::isEligibleForTailCallOptimization(
   const Function &CallerF = MF.getFunction();
   CallingConv::ID CallerCC = CallerF.getCallingConv();
 
+  // Arm64EC varargs calls expect that x4 = sp at entry, this complicates tail
+  // call handling so disable for now.
+  if (IsVarArg && Subtarget->isWindowsArm64EC())
+    return false;
+
   // SME Streaming functions are not eligible for TCO as they may require
   // the streaming mode or ZA to be restored after returning from the call.
   SMEAttrs CallerAttrs(MF.getFunction());
diff --git a/llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp b/llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
index 84057ea8d2214a..43a279de035ac7 100644
--- a/llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
+++ b/llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
@@ -909,6 +909,7 @@ bool AArch64CallLowering::isEligibleForTailCallOptimization(
   CallingConv::ID CalleeCC = Info.CallConv;
   MachineFunction &MF = MIRBuilder.getMF();
   const Function &CallerF = MF.getFunction();
+  const AArch64Subtarget &Subtarget = MF.getSubtarget<AArch64Subtarget>();
 
   LLVM_DEBUG(dbgs() << "Attempting to lower call as tail call\n");
 
@@ -926,6 +927,11 @@ bool AArch64CallLowering::isEligibleForTailCallOptimization(
     return false;
   }
 
+  // Arm64EC varargs calls expect that x4 = sp at entry, this complicates tail
+  // call handling so disable for now.
+  if (Info.IsVarArg && Subtarget.isWindowsArm64EC())
+    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).

@efriedma-quic
Copy link
Collaborator

Is this supposed to be applied on top of #79067 ?

Briefly testing, it seems like the existing logic works correctly: if the argument list fits into four or less integer registers, we tail-call with a "stack size" of zero, and if there are more arguments, we refuse to tail-call.

@bylaws
Copy link
Contributor Author

bylaws commented Jan 23, 2024

Is this supposed to be applied on top of #79067 ?

Briefly testing, it seems like the existing logic works correctly: if the argument list fits into four or less integer registers, we tail-call with a "stack size" of zero, and if there are more arguments, we refuse to tail-call.

The issue I ran into is that even when zero arguments are passed, the callee will still write the nonstack params to [x4-0x20,x4], so either x4 needs to point to 0x20 bytes of allocated stack space or the current SP, neither is the case in a tail call.

@bylaws
Copy link
Contributor Author

bylaws commented Jan 23, 2024

@efriedma-quic I should note there are several other fixes necessary for variadic calls in entry thunks to work properly, I pushed them a while back to bylaws@4ff9104 but now entry thunks are merged I'll clean them up and PR, since some of them are needed to merge this (guest exit thunk targets are incorrectly marked as varargs).

@efriedma-quic
Copy link
Collaborator

The issue I ran into is that even when zero arguments are passed, the callee will still write the nonstack params to [x4-0x20,x4], so either x4 needs to point to 0x20 bytes of allocated stack space or the current SP, neither is the case in a tail call.

"Current SP" is meaningful for a tail-call; it's just the SP on entry. I guess the current code doesn't compute that correctly, but it's very easy to fix; we do a similar computation elsewhere (see https://github.com/llvm/llvm-project/blob/6c98c5bd99b4c71a7895337cd4e437e023c2ec7a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp#L7951).

@bylaws
Copy link
Contributor Author

bylaws commented Jan 23, 2024

"Current SP" is meaningful for a tail-call; it's just the SP on entry. I guess the current code doesn't compute that correctly, but it's very easy to fix; we do a similar computation elsewhere (see https://github.com/llvm/llvm-project/blob/6c98c5bd99b4c71a7895337cd4e437e023c2ec7a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp#L7951).

Oh thanks for the pointer, will writeup a fix with that instead, assumed there was more too it since x86 disables them outright.

@bylaws bylaws closed this Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants