-
Notifications
You must be signed in to change notification settings - Fork 15.2k
release/20.x: [flang] fix AArch64 PCS for struct following pointer (#127802) #128518
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
Conversation
|
@tblah What do you think about merging this PR to the release branch? |
|
@llvm/pr-subscribers-flang-codegen @llvm/pr-subscribers-flang-fir-hlfir Author: None (llvmbot) ChangesBackport 449f84f Requested by: @DavidTruby Full diff: https://github.com/llvm/llvm-project/pull/128518.diff 2 Files Affected:
diff --git a/flang/lib/Optimizer/CodeGen/Target.cpp b/flang/lib/Optimizer/CodeGen/Target.cpp
index 1bc673bb34e32..2a1eb0bc33f5c 100644
--- a/flang/lib/Optimizer/CodeGen/Target.cpp
+++ b/flang/lib/Optimizer/CodeGen/Target.cpp
@@ -930,6 +930,13 @@ struct TargetAArch64 : public GenericTarget<TargetAArch64> {
.Case<fir::VectorType>([&](auto) {
TODO(loc, "passing vector argument to C by value is not supported");
return NRegs{};
+ })
+ .Default([&](auto ty) {
+ if (fir::conformsWithPassByRef(ty))
+ return NRegs{1, false}; // Pointers take 1 integer register
+ TODO(loc, "unsupported component type for BIND(C), VALUE derived "
+ "type argument");
+ return NRegs{};
});
}
diff --git a/flang/test/Fir/struct-passing-aarch64-byval.fir b/flang/test/Fir/struct-passing-aarch64-byval.fir
index 27143459dde2f..087efba393014 100644
--- a/flang/test/Fir/struct-passing-aarch64-byval.fir
+++ b/flang/test/Fir/struct-passing-aarch64-byval.fir
@@ -71,3 +71,17 @@ func.func private @too_many_hfa(!fir.type<hfa_max{i:f128,j:f128,k:f128,l:f128}>,
// CHECK-LABEL: func.func private @too_big(!fir.ref<!fir.type<too_big{i:!fir.array<5xi32>}>> {{{.*}}, llvm.byval = !fir.type<too_big{i:!fir.array<5xi32>}>})
func.func private @too_big(!fir.type<too_big{i:!fir.array<5xi32>}>)
+
+// CHECK-LABEL: func.func private @pointer_type(!fir.ref<i64>, !fir.array<1xi64>)
+func.func private @pointer_type(!fir.ref<i64>, !fir.type<pointer_type{i:i64}>)
+
+// CHECK-LABEL: func.func private @pointer_type_too_many_int(!fir.ref<i64>,
+// CHECK-SAME: !fir.array<2xi64>,
+// CHECK-SAME: !fir.array<2xi64>,
+// CHECK-SAME: !fir.array<2xi64>,
+// CHECK-SAME: !fir.ref<!fir.type<int_max{i:i64,j:i64}>> {{{.*}}, llvm.byval = !fir.type<int_max{i:i64,j:i64}>})
+func.func private @pointer_type_too_many_int(!fir.ref<i64>,
+ !fir.type<int_max{i:i64,j:i64}>,
+ !fir.type<int_max{i:i64,j:i64}>,
+ !fir.type<int_max{i:i64,j:i64}>,
+ !fir.type<int_max{i:i64,j:i64}>)
|
|
For context, this fixes an ICE in the specific scenario where you are trying to call a C function from Fortran that has both pointer and struct arguments, on AArch64. It’s quite niche but does cause a hard crash with a completely useless error message. I was hoping we could get it in to 20 just because the fix is very simple and since it only touches this specific scenario it's low risk too. |
|
This looks good to me but I am unsure what the exact interpretation of the rules should be.
This is not fixing a regression but I believe the patch is very low risk. I am unsure how "critical bug" is defined. Is anyone able to make an authoritative decision? |
nikic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backporting crash fixes is fine at this point, especially if they're simple and self-contained like this.
Pointers are already handled as taking up a register in the ABI handling, but the handling for structs was not taking this into account. This patch changes the struct handling to acknowledge that pointer arguments take up an integer register. Fixes llvm#123075 (cherry picked from commit 449f84f)
|
@DavidTruby (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR. |
Backport 449f84f
Requested by: @DavidTruby