Skip to content

[Aarch64] [ISel] Don't save vaargs registers if vaargs are unused #126780

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 5, 2025
Merged

[Aarch64] [ISel] Don't save vaargs registers if vaargs are unused #126780

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a84fa98
arm64: don't save vaargs regs if there is no va_start
pskrgag Feb 11, 2025
72ce44e
address review comments
pskrgag Feb 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8189,7 +8189,7 @@ SDValue AArch64TargetLowering::LowerFormalArguments(
}

// varargs
if (isVarArg) {
if (isVarArg && DAG.getMachineFunction().getFrameInfo().hasVAStart()) {
Copy link
Contributor

@arsenm arsenm Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this path is already handled in globalisel that should also be updated to match

Copy link
Contributor Author

@pskrgag pskrgag Feb 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I don't think I get what you mean, sorry.

IIUC, GlobalIsel is quite new approach for generating machine code, which is supposed to be a successor for SelectionDAG. It can be turned on using --global-isel=1.

So, I didn't find any hasVAStart() references in GISel code and tried simple examples to see if such optimization exists.

After some tests I've observed that llc actually crashes on vaargs, when GISel is turned on. Here is godbolt link for x86 https://godbolt.org/z/q66feEb65. And locally (after addressing @ostannard review) I got similar failure on CodeGen/AArch64/win64_vararg2.ll test.

+ /home/paskripkin/Documents/git/llvm-project/build/bin/llc -global-isel -mtriple=aarch64-pc-win32
+ /home/paskripkin/Documents/git/llvm-project/build/bin/FileCheck /home/paskripkin/Documents/git/llvm-project/llvm/test/CodeGen/AArch64/win64_vararg2.ll --check-prefix=GISEL
LLVM ERROR: cannot select: G_VASTART %17:gpr(p0) :: (store (s64) into %ir.valist) (in function: va_func)
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /home/paskripkin/Documents/git/llvm-project/build/bin/llc -global-isel -mtriple=aarch64-pc-win32
1.	Running pass 'Function Pass Manager' on module '<stdin>'.
2.	Running pass 'InstructionSelect' on function '@va_func'

Is it known? If not, I will try to look deeper

Copy link
Collaborator

@efriedma-quic efriedma-quic Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it works with -global-isel-abort=0, you're fine; you don't need to implement G_VASTART on any target where it isn't already implemented.

Copy link
Contributor Author

@pskrgag pskrgag Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Added -global-isel-abort=0 to failing test.

if (!Subtarget->isTargetDarwin() || IsWin64) {
// The AAPCS variadic function ABI is identical to the non-variadic
// one. As a result there may be more arguments in registers and we should
Expand Down
18 changes: 4 additions & 14 deletions llvm/test/CodeGen/AArch64/GlobalISel/aapcs_vararg_frame.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,12 @@
; RUN: llc < %s --global-isel=1 -mtriple=aarch64-linux-gnu -mattr=+fp-armv8 | FileCheck %s --check-prefix=GISEL

define void @va(i32 %count, half %f, ...) nounwind {
; CHECK-LABEL: va:
; CHECK: // %bb.0: // %entry
; CHECK-NEXT: sub sp, sp, #176
; CHECK-NEXT: stp x2, x3, [sp, #128]
; CHECK-NEXT: str x1, [sp, #120]
; CHECK-NEXT: stp x4, x5, [sp, #144]
; CHECK-NEXT: stp x6, x7, [sp, #160]
; CHECK-NEXT: stp q1, q2, [sp]
; CHECK-NEXT: stp q3, q4, [sp, #32]
; CHECK-NEXT: stp q5, q6, [sp, #64]
; CHECK-NEXT: str q7, [sp, #96]
; CHECK-NEXT: add sp, sp, #176
; CHECK-LABEL: va: // @va
; CHECK: // %bb.0: // %entry
; CHECK-NEXT: ret
;
; GISEL-LABEL: va:
; GISEL: // %bb.0: // %entry
; GISEL-LABEL: va: // @va
; GISEL: // %bb.0: // %entry
; GISEL-NEXT: sub sp, sp, #176
; GISEL-NEXT: stp x1, x2, [sp, #120]
; GISEL-NEXT: stp x3, x4, [sp, #136]
Expand Down
13 changes: 7 additions & 6 deletions llvm/test/CodeGen/AArch64/alloca.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,21 +82,22 @@ define void @test_variadic_alloca(i64 %n, ...) {
; [...]
; CHECK-DAG: stp q0, q1, [x29, #-192]

; CHECK-DAG: stp x5, x6, [x29, #-24]
; CHECK-DAG: stp x5, x6, [x29, #-32]
; [...]
; CHECK-DAG: stp x1, x2, [x29, #-56]
; CHECK-DAG: stp x1, x2, [x29, #-64]

; CHECK-NOFP-ARM64: stp x29, x30, [sp, #-16]!
; CHECK-NOFP-ARM64: mov x29, sp
; CHECK-NOFP-ARM64: sub sp, sp, #64
; CHECK-NOFP-ARM64-DAG: stp x5, x6, [x29, #-24]
; CHECK-NOFP-ARM64-DAG: stp x5, x6, [x29, #-32]
; [...]
; CHECK-NOFP-ARM64-DAG: stp x3, x4, [x29, #-40]
; CHECK-NOFP-ARM64-DAG: stp x3, x4, [x29, #-48]
; [...]
; CHECK-NOFP-ARM64-DAG: stp x1, x2, [x29, #-56]
; CHECK-NOFP-ARM64-DAG: stp x1, x2, [x29, #-64]
; [...]
; CHECK-NOFP-ARM64: mov x8, sp

%valist = alloca i8
call void @llvm.va_start(ptr %valist)
%addr = alloca i8, i64 %n

call void @use_addr(ptr %addr)
Expand Down
4 changes: 0 additions & 4 deletions llvm/test/CodeGen/AArch64/arm64ec-hybrid-patchable.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@ define void @has_varargs(...) hybrid_patchable nounwind {
; CHECK-NEXT: .p2align 2
; CHECK-NEXT: "#has_varargs$hp_target": // @"#has_varargs$hp_target"
; CHECK-NEXT: // %bb.0:
; CHECK-NEXT: sub sp, sp, #32
; CHECK-NEXT: stp x0, x1, [x4, #-32]
; CHECK-NEXT: stp x2, x3, [x4, #-16]
; CHECK-NEXT: add sp, sp, #32
; CHECK-NEXT: ret
ret void
}
Expand Down
4 changes: 2 additions & 2 deletions llvm/test/CodeGen/AArch64/darwinpcs-tail.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
; CHECK-LABEL: _tailTest:
; CHECK: b __ZN1C3addEPKcz
; CHECK-LABEL: __ZThn8_N1C1fEiiiiiiiiiz:
; CHECK: ldr w9, [sp, #4]
; CHECK: str w9, [sp, #4]
; CHECK: ldr w8, [sp, #4]
; CHECK: str w8, [sp, #4]
; CHECK: b __ZN1C1fEiiiiiiiiiz

%class.C = type { %class.A.base, [4 x i8], %class.B.base, [4 x i8] }
Expand Down
6 changes: 4 additions & 2 deletions llvm/test/CodeGen/AArch64/vararg-tallcall.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ $"??_9B@@$BA@AA" = comdat any
; Function Attrs: noinline optnone
define linkonce_odr void @"??_9B@@$BA@AA"(ptr %this, ...) #1 comdat align 2 {
entry:
%valist = alloca i8
call void @llvm.va_start.p0(ptr %valist)
%this.addr = alloca ptr, align 8
store ptr %this, ptr %this.addr, align 8
%this1 = load ptr, ptr %this.addr, align 8
Expand All @@ -37,6 +39,6 @@ attributes #1 = { noinline optnone "thunk" }
; CHECK-EC: ldr x9, [x0]
; CHECK-EC: ldr x11, [x9]
; CHECK-EC: mov v0.16b, v7.16b
; CHECK-EC: add x4, sp, #64
; CHECK-EC: add sp, sp, #64
; CHECK-EC: add x4, sp, #80
; CHECK-EC: add sp, sp, #80
; CHECK-EC: br x11
11 changes: 4 additions & 7 deletions llvm/test/CodeGen/AArch64/win64_vararg2.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,14 @@ define i1 @va_func(i32 %a, i8 %b, i8 %c, ...) {
; CHECK-LABEL: va_func:
; CHECK: .seh_proc va_func
; CHECK-NEXT: // %bb.0:
; CHECK-NEXT: sub sp, sp, #80
Copy link
Collaborator

@ostannard ostannard Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another test which is trying to check vararg behaviour, so should have a va_start call.

Copy link
Contributor Author

@pskrgag pskrgag Feb 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So looks like GISel does not support vastart for win calling convention

bool AArch64InstructionSelector::selectVaStartAAPCS(
    MachineInstr &I, MachineFunction &MF, MachineRegisterInfo &MRI) const {

  if (STI.isCallingConvWin64(MF.getFunction().getCallingConv(),
                             MF.getFunction().isVarArg()))
    return false;

; CHECK-NEXT: .seh_stackalloc 80
; CHECK-NEXT: sub sp, sp, #32
; CHECK-NEXT: .seh_stackalloc 32
; CHECK-NEXT: str x19, [sp, #16] // 8-byte Folded Spill
; CHECK-NEXT: .seh_save_reg x19, 16
; CHECK-NEXT: str x30, [sp, #24] // 8-byte Folded Spill
; CHECK-NEXT: .seh_save_reg x30, 24
; CHECK-NEXT: .seh_endprologue
; CHECK-NEXT: mov w19, w0
; CHECK-NEXT: stp x3, x4, [sp, #40]
; CHECK-NEXT: stp x5, x6, [sp, #56]
; CHECK-NEXT: str x7, [sp, #72]
; CHECK-NEXT: str w0, [sp, #12]
; CHECK-NEXT: strb w1, [sp, #11]
; CHECK-NEXT: strb w2, [sp, #10]
Expand All @@ -29,8 +26,8 @@ define i1 @va_func(i32 %a, i8 %b, i8 %c, ...) {
; CHECK-NEXT: .seh_save_reg x30, 24
; CHECK-NEXT: ldr x19, [sp, #16] // 8-byte Folded Reload
; CHECK-NEXT: .seh_save_reg x19, 16
; CHECK-NEXT: add sp, sp, #80
; CHECK-NEXT: .seh_stackalloc 80
; CHECK-NEXT: add sp, sp, #32
; CHECK-NEXT: .seh_stackalloc 32
; CHECK-NEXT: .seh_endepilogue
; CHECK-NEXT: ret
; CHECK-NEXT: .seh_endfunclet
Expand Down