Repository navigation
Conversation
getThunkArgTypes gave every ordinary parameter alignment 1, so types with different native alignment (e.g. i128 vs [2 x i64]) mapped to the same thunk name and shared a thunk that reads the wrong argument registers. Use the DataLayout ABI alignment so the names differ. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Hello @brenocfg 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
|
@llvm/pr-subscribers-backend-aarch64 Author: Breno Campos (brenocfg) Changes
Full diff: https://github.com/llvm/llvm-project/pull/238613.diff 4 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp b/llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp
index 115ab144f3c15..1d6c491e1d029 100644
--- a/llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp
@@ -206,7 +206,7 @@ void AArch64Arm64ECCallLowering::getThunkArgTypes(
Align ParamAlign = AttrList.getParamAlignment(I).valueOrOne();
#else
uint64_t ArgSizeBytes = 0;
- Align ParamAlign = Align();
+ Align ParamAlign = M->getDataLayout().getABITypeAlign(FT->getParamType(I));
#endif
auto [Arm64Ty, X64Ty, ArgTranslation] =
canonicalizeThunkType(FT->getParamType(I), ParamAlign,
diff --git a/llvm/test/CodeGen/AArch64/arm64ec-aggregate-alignment.ll b/llvm/test/CodeGen/AArch64/arm64ec-aggregate-alignment.ll
new file mode 100644
index 0000000000000..bd105526c5222
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/arm64ec-aggregate-alignment.ll
@@ -0,0 +1,51 @@
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -O0 -stop-after=Arm64ECCallLowering %s -o - | FileCheck %s --check-prefix=IR
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -O2 -stop-after=Arm64ECCallLowering %s -o - | FileCheck %s --check-prefix=IR
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -O2 %s -o - | FileCheck %s --check-prefix=ASM
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -O0 -verify-machineinstrs -filetype=obj %s -o %t.O0.obj
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -O2 -verify-machineinstrs -filetype=obj %s -o %t.O2.obj
+
+; These 16-byte aggregates need different native argument registers after a
+; pointer. Their entry and exit thunks must not share size-only names.
+define void @aligned_store(ptr %out, i128 %value) {
+ store i128 %value, ptr %out, align 16
+ ret void
+}
+
+define void @pair_store(ptr %out, [2 x i64] %value) {
+ store [2 x i64] %value, ptr %out, align 8
+ ret void
+}
+
+define void @call_aligned(ptr %callee, ptr %out, i128 %value) {
+ call void %callee(ptr %out, i128 %value)
+ ret void
+}
+
+define void @call_pair(ptr %callee, ptr %out, [2 x i64] %value) {
+ call void %callee(ptr %out, [2 x i64] %value)
+ ret void
+}
+
+; IR-LABEL: define linkonce_odr {{.*}}void @"$iexit_thunk$cdecl$v$i8m16a16"(
+; IR: store i128 {{.*}}, ptr %[[ALIGNED_SLOT:[^ ,]+]], align 16
+; IR-NEXT: call cc108 void {{.*}}(ptr {{.*}}, i64 {{.*}}, ptr %[[ALIGNED_SLOT]])
+; IR-NEXT: ret void
+; IR-LABEL: define linkonce_odr {{.*}}void @"$iexit_thunk$cdecl$v$i8m16"(
+; IR: store [2 x i64] {{.*}}, ptr %[[PAIR_SLOT:[^ ,]+]], align 8
+; IR-NEXT: call cc108 void {{.*}}(ptr {{.*}}, i64 {{.*}}, ptr %[[PAIR_SLOT]])
+; IR-NEXT: ret void
+; IR-LABEL: define linkonce_odr {{.*}}void @"$ientry_thunk$cdecl$v$i8m16a16"(
+; IR: %[[ALIGNED_ARG:[^ ]+]] = load i128, ptr {{.*}}, align 16
+; IR-NEXT: call void {{.*}}(i64 {{.*}}, i128 %[[ALIGNED_ARG]])
+; IR-NEXT: ret void
+; IR-LABEL: define linkonce_odr {{.*}}void @"$ientry_thunk$cdecl$v$i8m16"(
+; IR: %[[PAIR_ARG:[^ ]+]] = load [2 x i64], ptr {{.*}}, align 8
+; IR-NEXT: call void {{.*}}(i64 {{.*}}, [2 x i64] %[[PAIR_ARG]])
+; IR-NEXT: ret void
+
+; ASM-LABEL: $ientry_thunk$cdecl$v$i8m16a16:
+; ASM: ldp x2, x3, [x1]
+; ASM-NEXT: blr x9
+; ASM-LABEL: $ientry_thunk$cdecl$v$i8m16:
+; ASM: ldp x1, x2, [x1]
+; ASM-NEXT: blr x9
diff --git a/llvm/test/CodeGen/AArch64/arm64ec-entry-thunks.ll b/llvm/test/CodeGen/AArch64/arm64ec-entry-thunks.ll
index eafb401a1f705..f78c191c67f78 100644
--- a/llvm/test/CodeGen/AArch64/arm64ec-entry-thunks.ll
+++ b/llvm/test/CodeGen/AArch64/arm64ec-entry-thunks.ll
@@ -781,8 +781,8 @@ start:
}
define <8 x i16> @large_vector(<8 x i16> %0) {
-; CHECK-LABEL: .def $ientry_thunk$cdecl$m16$m16;
-; CHECK: .section .wowthk$aa,"xr",discard,$ientry_thunk$cdecl$m16$m16
+; CHECK-LABEL: .def $ientry_thunk$cdecl$m16$m16a16;
+; CHECK: .section .wowthk$aa,"xr",discard,$ientry_thunk$cdecl$m16$m16a16
; CHECK: // %bb.0:
; CHECK-NEXT: stp q6, q7, [sp, #-192]! // 32-byte Folded Spill
; CHECK-NEXT: .seh_save_any_reg_px q6, 192
@@ -885,5 +885,5 @@ start:
; CHECK-NEXT: .symidx $ientry_thunk$cdecl$m$m
; CHECK-NEXT: .word 1
; CHECK-NEXT: .symidx "#large_vector"
-; CHECK-NEXT: .symidx $ientry_thunk$cdecl$m16$m16
+; CHECK-NEXT: .symidx $ientry_thunk$cdecl$m16$m16a16
; CHECK-NEXT: .word 1
diff --git a/llvm/test/CodeGen/AArch64/arm64ec-exit-thunks.ll b/llvm/test/CodeGen/AArch64/arm64ec-exit-thunks.ll
index 78b59cfa196d8..60230af11d285 100644
--- a/llvm/test/CodeGen/AArch64/arm64ec-exit-thunks.ll
+++ b/llvm/test/CodeGen/AArch64/arm64ec-exit-thunks.ll
@@ -624,8 +624,8 @@ declare void @has_varargs_sret(ptr sret([100 x i8]), ...) nounwind;
; CHECK-NEXT: .seh_endfunclet
; CHECK-NEXT: .seh_endproc
declare [2 x i8] @small_array([2 x i8], [2 x half], [2 x fp128], [2 x float]) nounwind;
-; CHECK-LABEL: .def $iexit_thunk$cdecl$m2$m2__llvm_H__4__llvm_Q__32F8;
-; CHECK: .section .wowthk$aa,"xr",discard,$iexit_thunk$cdecl$m2$m2__llvm_H__4__llvm_Q__32F8
+; CHECK-LABEL: .def $iexit_thunk$cdecl$m2$m2__llvm_H__4__llvm_Q__32a16F8;
+; CHECK: .section .wowthk$aa,"xr",discard,$iexit_thunk$cdecl$m2$m2__llvm_H__4__llvm_Q__32a16F8
; CHECK: // %bb.0:
; CHECK-NEXT: sub sp, sp, #112
; CHECK-NEXT: .seh_stackalloc 112
@@ -671,8 +671,8 @@ declare [2 x i8] @small_array([2 x i8], [2 x half], [2 x fp128], [2 x float]) no
; CHECK-NEXT: adrp x11, small_array
; CHECK-NEXT: add x11, x11, :lo12:small_array
; CHECK-NEXT: ldr x8, [x8, :lo12:__os_arm64x_check_icall]
-; CHECK-NEXT: adrp x10, $iexit_thunk$cdecl$m2$m2__llvm_H__4__llvm_Q__32F8
-; CHECK-NEXT: add x10, x10, :lo12:$iexit_thunk$cdecl$m2$m2__llvm_H__4__llvm_Q__32F8
+; CHECK-NEXT: adrp x10, $iexit_thunk$cdecl$m2$m2__llvm_H__4__llvm_Q__32a16F8
+; CHECK-NEXT: add x10, x10, :lo12:$iexit_thunk$cdecl$m2$m2__llvm_H__4__llvm_Q__32a16F8
; CHECK-NEXT: blr x8
; CHECK-NEXT: .seh_startepilogue
; CHECK-NEXT: ldr x30, [sp], #16 // 8-byte Folded Reload
@@ -857,8 +857,8 @@ declare <4 x i8> @small_vector(<4 x i8> %0) nounwind;
; CHECK-NEXT: .seh_endproc
declare <8 x i16> @large_vector(<8 x i16> %0) nounwind;
-; CHECK-LABEL: .def $iexit_thunk$cdecl$m16$m16;
-; CHECK: .section .wowthk$aa,"xr",discard,$iexit_thunk$cdecl$m16$m16
+; CHECK-LABEL: .def $iexit_thunk$cdecl$m16$m16a16;
+; CHECK: .section .wowthk$aa,"xr",discard,$iexit_thunk$cdecl$m16$m16a16
; CHECK: // %bb.0:
; CHECK-NEXT: sub sp, sp, #80
; CHECK-NEXT: .seh_stackalloc 80
@@ -895,8 +895,8 @@ declare <8 x i16> @large_vector(<8 x i16> %0) nounwind;
; CHECK-NEXT: adrp x11, large_vector
; CHECK-NEXT: add x11, x11, :lo12:large_vector
; CHECK-NEXT: ldr x8, [x8, :lo12:__os_arm64x_check_icall]
-; CHECK-NEXT: adrp x10, $iexit_thunk$cdecl$m16$m16
-; CHECK-NEXT: add x10, x10, :lo12:$iexit_thunk$cdecl$m16$m16
+; CHECK-NEXT: adrp x10, $iexit_thunk$cdecl$m16$m16a16
+; CHECK-NEXT: add x10, x10, :lo12:$iexit_thunk$cdecl$m16$m16a16
; CHECK-NEXT: blr x8
; CHECK-NEXT: .seh_startepilogue
; CHECK-NEXT: ldr x30, [sp], #16 // 8-byte Folded Reload
@@ -1012,7 +1012,7 @@ declare void @"??@md5mangleaaaaaaaaaaaaaaaaaaaaaaa@"()
; CHECK-NEXT: .symidx has_varargs_sret
; CHECK-NEXT: .word 0
; CHECK-NEXT: .symidx small_array
-; CHECK-NEXT: .symidx $iexit_thunk$cdecl$m2$m2__llvm_H__4__llvm_Q__32F8
+; CHECK-NEXT: .symidx $iexit_thunk$cdecl$m2$m2__llvm_H__4__llvm_Q__32a16F8
; CHECK-NEXT: .word 4
; CHECK-NEXT: .symidx "#small_array$exit_thunk"
; CHECK-NEXT: .symidx small_array
@@ -1036,7 +1036,7 @@ declare void @"??@md5mangleaaaaaaaaaaaaaaaaaaaaaaa@"()
; CHECK-NEXT: .symidx small_vector
; CHECK-NEXT: .word 0
; CHECK-NEXT: .symidx large_vector
-; CHECK-NEXT: .symidx $iexit_thunk$cdecl$m16$m16
+; CHECK-NEXT: .symidx $iexit_thunk$cdecl$m16$m16a16
; CHECK-NEXT: .word 4
; CHECK-NEXT: .symidx "#large_vector$exit_thunk"
; CHECK-NEXT: .symidx large_vector
|
| ; RUN: llc -mtriple=arm64ec-pc-windows-msvc -O2 -stop-after=Arm64ECCallLowering %s -o - | FileCheck %s --check-prefix=IR | ||
| ; RUN: llc -mtriple=arm64ec-pc-windows-msvc -O2 %s -o - | FileCheck %s --check-prefix=ASM | ||
| ; RUN: llc -mtriple=arm64ec-pc-windows-msvc -O0 -verify-machineinstrs -filetype=obj %s -o %t.O0.obj | ||
| ; RUN: llc -mtriple=arm64ec-pc-windows-msvc -O2 -verify-machineinstrs -filetype=obj %s -o %t.O2.obj |
There was a problem hiding this comment.
This is a lot of RUN lines. Can you extend the existing test files instead of adding a new one?
There was a problem hiding this comment.
Fixed! Moved them to the entry/exit thunk test files.
| #else | ||
| uint64_t ArgSizeBytes = 0; | ||
| Align ParamAlign = Align(); | ||
| Align ParamAlign = M->getDataLayout().getABITypeAlign(FT->getParamType(I)); |
There was a problem hiding this comment.
The only argument that we're likely to see with alignment larger than 8 is an i128? I guess this is fine, then.
|
Also, are you looking at Arm64EC ABI in general? I'd like to coordinate if you are. |
… tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Not the whole ABI in general, but I have been looking into the ABI's behaviour with x86 intrinsics specifically, which is what led to finding this bug. I have a couple of other branches internally that address ARM64EC x86 intrinsic compatibility like header mappings and support for MSVC's |
getThunkArgTypesgave every ordinary parameter alignment 1, so types with different native alignment (e.g.i128vs[2 x i64]) mapped to the same thunk name and shared a thunk that reads the wrong argument registers. This PR makes it use theDataLayoutABI alignment so the names differ and the right thunk is used.