Skip to content

[AArch64][GISel] Incorrect ABI for calls with tail marker #70207

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

Closed
nikic opened this issue Oct 25, 2023 · 5 comments · Fixed by #70215 or llvm/llvm-project-release-prs#760
Closed

[AArch64][GISel] Incorrect ABI for calls with tail marker #70207

nikic opened this issue Oct 25, 2023 · 5 comments · Fixed by #70215 or llvm/llvm-project-release-prs#760

Comments

@nikic
Copy link
Contributor

nikic commented Oct 25, 2023

; RUN: llc -O0 -mtriple=aarch64-unknown-linux-gnu < %s | FileCheck %s
declare void @func(i64, i64, i64, i64, i64, i128, i128)

define void @test(i128 %arg1, i128 %arg2) nounwind {
  tail call void @func(i64 0, i64 0, i64 0, i64 0, i64 0, i128 %arg1, i128 %arg2)
  ret void
}

This produces:

	sub	sp, sp, #64
	str	x30, [sp, #48]                  // 8-byte Folded Spill
	mov	x6, x0
	mov	x8, sp
	str	x1, [x8]
	str	x2, [x8, #16]
	str	x3, [x8, #32]
	mov	x4, xzr
	mov	x0, x4
	mov	x1, x4
	mov	x2, x4
	mov	x3, x4
	bl	func
	ldr	x30, [sp, #48]                  // 8-byte Folded Reload
	add	sp, sp, #64
	ret

Note that both of the i128 arguments get passed on the stack. Compare with the -global-isel=0 output:

	sub	sp, sp, #32
	str	x30, [sp, #16]                  // 8-byte Folded Spill
	mov	x7, x1
	mov	x6, x0
	mov	x8, sp
	str	x3, [x8, #8]
	str	x2, [x8]
	mov	x4, xzr
	mov	x0, x4
	mov	x1, x4
	mov	x2, x4
	mov	x3, x4
	bl	func
	ldr	x30, [sp, #16]                  // 8-byte Folded Reload
	add	sp, sp, #32
	ret

In this case the first i128 argument is passed in registers and only the second one on the stack.

Peculiarly, this does not happen when dropping the tail marker (which shouldn't make a difference as no actual tail call gets generated).

@llvmbot
Copy link
Member

llvmbot commented Oct 25, 2023

@llvm/issue-subscribers-backend-aarch64

Author: Nikita Popov (nikic)

```llvm ; RUN: llc -O0 -mtriple=aarch64-unknown-linux-gnu < %s | FileCheck %s declare void @func(i64, i64, i64, i64, i64, i128, i128)

define void @test(i128 %arg1, i128 %arg2) nounwind {
tail call void @func(i64 0, i64 0, i64 0, i64 0, i64 0, i128 %arg1, i128 %arg2)
ret void
}

This produces:
```asm
	sub	sp, sp, #<!-- -->64
	str	x30, [sp, #<!-- -->48]                  // 8-byte Folded Spill
	mov	x6, x0
	mov	x8, sp
	str	x1, [x8]
	str	x2, [x8, #<!-- -->16]
	str	x3, [x8, #<!-- -->32]
	mov	x4, xzr
	mov	x0, x4
	mov	x1, x4
	mov	x2, x4
	mov	x3, x4
	bl	func
	ldr	x30, [sp, #<!-- -->48]                  // 8-byte Folded Reload
	add	sp, sp, #<!-- -->64
	ret

Note that both of the i128 arguments get passed on the stack. Compare with the -global-isel=0 output:

	sub	sp, sp, #<!-- -->32
	str	x30, [sp, #<!-- -->16]                  // 8-byte Folded Spill
	mov	x7, x1
	mov	x6, x0
	mov	x8, sp
	str	x3, [x8, #<!-- -->8]
	str	x2, [x8]
	mov	x4, xzr
	mov	x0, x4
	mov	x1, x4
	mov	x2, x4
	mov	x3, x4
	bl	func
	ldr	x30, [sp, #<!-- -->16]                  // 8-byte Folded Reload
	add	sp, sp, #<!-- -->32
	ret

In this case the first i128 argument is passed in registers and only the second one on the stack.

Peculiarly, this does not happen when dropping the tail marker (which shouldn't make a difference as no actual tail call gets generated).

@nikic
Copy link
Contributor Author

nikic commented Oct 25, 2023

The problem seems to be that

if (!determineAssignments(CalleeAssigner, OutArgs, OutInfo)) {
when trying to determine whether a tail call is legal modifies state in some way -- looking at that function, it at least modifies the argument flags.

@nikic nikic self-assigned this Oct 25, 2023
nikic added a commit that referenced this issue Oct 25, 2023
@tru tru moved this from Needs Triage to Needs Review in LLVM Release Status Oct 27, 2023
@nikic nikic reopened this Oct 30, 2023
@nikic nikic closed this as completed in 292f34b Oct 30, 2023
@nikic nikic reopened this Oct 30, 2023
@nikic
Copy link
Contributor Author

nikic commented Oct 30, 2023

/cherry-pick d9cfb82 292f34b

@llvmbot
Copy link
Member

llvmbot commented Oct 30, 2023

/branch llvm/llvm-project-release-prs/issue70207

llvmbot pushed a commit to llvm/llvm-project-release-prs that referenced this issue Oct 30, 2023
…(#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 llvm/llvm-project#70207.

(cherry picked from commit 292f34b0d3cb2a04be5ebb85aaeb838b29f71323)
@llvmbot
Copy link
Member

llvmbot commented Oct 30, 2023

/pull-request llvm/llvm-project-release-prs#760

tru pushed a commit to llvm/llvm-project-release-prs that referenced this issue Oct 31, 2023
…(#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 llvm/llvm-project#70207.

(cherry picked from commit 292f34b0d3cb2a04be5ebb85aaeb838b29f71323)
@tru tru moved this from Needs Review to Done in LLVM Release Status Oct 31, 2023
tru pushed a commit that referenced this issue Oct 31, 2023
(cherry picked from commit d9cfb82)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment