[AArch64][GlobalISel] Fix incorrect ABI when tail call not supported #70215
Conversation
The check for whether a tail call is supported calls determineAssignments(), which may modify argument flags. As such, even though the check fails and a non-tail call will be emitted, it will not have a different (incorrect) ABI. Fix this by operating on a separate copy of the arguments.
|
@llvm/pr-subscribers-backend-aarch64 @llvm/pr-subscribers-llvm-globalisel Author: Nikita Popov (nikic) ChangesThe check for whether a tail call is supported calls determineAssignments(), which may modify argument flags. As such, even though the check fails and a non-tail call will be emitted, it will not have a different (incorrect) ABI. Fix this by operating on a separate copy of the arguments. Fixes #70207. Full diff: https://github.com/llvm/llvm-project/pull/70215.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp b/llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
index 2d6cc870f98e77f..02a0c58c8fd0f64 100644
--- a/llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
+++ b/llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
@@ -854,7 +854,10 @@ bool AArch64CallLowering::areCalleeOutgoingArgsTailCallable(
AArch64OutgoingValueAssigner CalleeAssigner(AssignFnFixed, AssignFnVarArg,
Subtarget, /*IsReturn*/ false);
- if (!determineAssignments(CalleeAssigner, OutArgs, OutInfo)) {
+ // determineAssignments() may modify argument flags, so make a copy.
+ SmallVector<ArgInfo, 8> OutArgsCopy;
+ append_range(OutArgsCopy, OutArgs);
+ if (!determineAssignments(CalleeAssigner, OutArgsCopy, OutInfo)) {
LLVM_DEBUG(dbgs() << "... Could not analyze call operands.\n");
return false;
}
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/call-lowering-tail-call-fallback.ll b/llvm/test/CodeGen/AArch64/GlobalISel/call-lowering-tail-call-fallback.ll
index fc6eefb4016b66d..ebd2beca6781050 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/call-lowering-tail-call-fallback.ll
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/call-lowering-tail-call-fallback.ll
@@ -3,30 +3,26 @@
declare void @func(i64, i64, i64, i64, i64, i128, i128)
-; FIXME: This is a miscompile.
; Make sure the check for whether a tail call is allowed does not affect the
; calling convention if it fails.
; The first i128 argument should be passed in registers, not on the stack.
define void @pr70207(i128 %arg1, i128 %arg2) nounwind {
; CHECK-LABEL: pr70207:
; CHECK: // %bb.0:
-; CHECK-NEXT: sub sp, sp, #64
+; CHECK-NEXT: mov x8, x2
; CHECK-NEXT: mov x6, x0
-; CHECK-NEXT: mov x8, x1
-; CHECK-NEXT: mov x9, x2
-; CHECK-NEXT: mov x10, x3
+; CHECK-NEXT: mov x7, x1
+; CHECK-NEXT: mov x9, x3
; CHECK-NEXT: mov x0, xzr
; CHECK-NEXT: mov x1, xzr
; CHECK-NEXT: mov x2, xzr
; CHECK-NEXT: mov x3, xzr
; CHECK-NEXT: mov x4, xzr
-; CHECK-NEXT: str x30, [sp, #48] // 8-byte Folded Spill
-; CHECK-NEXT: str x8, [sp]
-; CHECK-NEXT: str x9, [sp, #16]
-; CHECK-NEXT: str x10, [sp, #32]
+; CHECK-NEXT: str x8, [sp, #-32]!
+; CHECK-NEXT: stp x9, x30, [sp, #8] // 8-byte Folded Spill
; CHECK-NEXT: bl func
-; CHECK-NEXT: ldr x30, [sp, #48] // 8-byte Folded Reload
-; CHECK-NEXT: add sp, sp, #64
+; CHECK-NEXT: ldr x30, [sp, #16] // 8-byte Folded Reload
+; CHECK-NEXT: add sp, sp, #32
; CHECK-NEXT: ret
tail call void @func(i64 0, i64 0, i64 0, i64 0, i64 0, i128 %arg1, i128 %arg2)
ret void
|
|
The documentation of |
I don't think we have to be too dogmatic about that. Unnecessary array copies also (minutely) impact compile time which we'd be paying the cost of on every call to |
|
Just to clarify, determineAssignments() does succeed here, it's just that following checks fail and the function has already changed state. Just a boolean canDetermineAssignments() query wouldn't be enough for use in this function, we do need the result as well. |
| // determineAssignments() may modify argument flags, so make a copy. | ||
| SmallVector<ArgInfo, 8> OutArgsCopy; | ||
| append_range(OutArgsCopy, OutArgs); | ||
| if (!determineAssignments(CalleeAssigner, OutArgsCopy, OutInfo)) { | ||
| LLVM_DEBUG(dbgs() << "... Could not analyze call operands.\n"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Just a boolean canDetermineAssignments() query wouldn't be enough for use in this function, we do need the result as well.
I interpret this to mean that the mutations performed by determineAssignments are necessary when this function succeeds.
But in that case, I would expect to see *OutArgs = OutArgsCopy; or similar at the end of this function?
There was a problem hiding this comment.
What I was referring to here is that OutInfo is used by further checks in this function. Though after your comment I saw that OutArgs is also used, so I've tweaked the code to make sure it gets the modified version, not the original one (I'm not sure whether this actually matters here, but better safe than sorry).
We don't need the result of determineAssignments() to persist outside this function (that is, the areCalleeOutgoingArgsTailCallable function). The actual tail call emission will call determineAssignments() again.
…70215) The check for whether a tail call is supported calls determineAssignments(), which may modify argument flags. As such, even though the check fails and a non-tail call will be emitted, it will not have a different (incorrect) ABI. Fix this by operating on a separate copy of the arguments. Fixes #70207. (cherry picked from commit 292f34b)
The check for whether a tail call is supported calls determineAssignments(), which may modify argument flags. As such, even though the check fails and a non-tail call will be emitted, it will not have a different (incorrect) ABI.
Fix this by operating on a separate copy of the arguments.
Fixes #70207.