Fix codegen for transparent_union function params #104816
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lei137
requested review from
ahatanak,
amy-kwan,
chenzheng1030,
efriedma-quic,
hubert-reinterpretcast and
s-barannikov
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-backend-aarch64 @llvm/pr-subscribers-clang Author: Lei Huang (lei137) ChangesUpdate codegen for func param with transparent_union attr to be that of This is a followup to #101738 to fix non-ppc codegen and closes #76773. Full diff: https://github.com/llvm/llvm-project/pull/104816.diff 6 Files Affected:
diff --git a/clang/lib/CodeGen/Targets/AArch64.cpp b/clang/lib/CodeGen/Targets/AArch64.cpp
index 97381f673c2849..2f119feb93aaf3 100644
--- a/clang/lib/CodeGen/Targets/AArch64.cpp
+++ b/clang/lib/CodeGen/Targets/AArch64.cpp
@@ -304,7 +304,7 @@ AArch64ABIInfo::classifyArgumentType(QualType Ty, bool IsVariadic,
return getNaturalAlignIndirect(Ty, false);
return (isPromotableIntegerTypeForABI(Ty) && isDarwinPCS()
- ? ABIArgInfo::getExtend(Ty)
+ ? ABIArgInfo::getExtend(Ty, CGT.ConvertType(Ty))
: ABIArgInfo::getDirect());
}
diff --git a/clang/lib/CodeGen/Targets/ARM.cpp b/clang/lib/CodeGen/Targets/ARM.cpp
index d032b88d7683cd..f7d7471d386b21 100644
--- a/clang/lib/CodeGen/Targets/ARM.cpp
+++ b/clang/lib/CodeGen/Targets/ARM.cpp
@@ -354,8 +354,9 @@ ABIArgInfo ARMABIInfo::classifyArgumentType(QualType Ty, bool isVariadic,
if (EIT->getNumBits() > 64)
return getNaturalAlignIndirect(Ty, /*ByVal=*/true);
- return (isPromotableIntegerTypeForABI(Ty) ? ABIArgInfo::getExtend(Ty)
- : ABIArgInfo::getDirect());
+ return (isPromotableIntegerTypeForABI(Ty)
+ ? ABIArgInfo::getExtend(Ty, CGT.ConvertType(Ty))
+ : ABIArgInfo::getDirect());
}
if (CGCXXABI::RecordArgABI RAA = getRecordArgABI(Ty, getCXXABI())) {
diff --git a/clang/lib/CodeGen/Targets/RISCV.cpp b/clang/lib/CodeGen/Targets/RISCV.cpp
index 826a1ec2c9d386..57b09f1a3d7632 100644
--- a/clang/lib/CodeGen/Targets/RISCV.cpp
+++ b/clang/lib/CodeGen/Targets/RISCV.cpp
@@ -51,7 +51,7 @@ class RISCVABIInfo : public DefaultABIInfo {
RValue EmitVAArg(CodeGenFunction &CGF, Address VAListAddr, QualType Ty,
AggValueSlot Slot) const override;
- ABIArgInfo extendType(QualType Ty) const;
+ ABIArgInfo extendType(QualType Ty, llvm::Type *CoerceTy = nullptr) const;
bool detectFPCCEligibleStruct(QualType Ty, llvm::Type *&Field1Ty,
CharUnits &Field1Off, llvm::Type *&Field2Ty,
@@ -439,12 +439,12 @@ ABIArgInfo RISCVABIInfo::classifyArgumentType(QualType Ty, bool IsFixed,
// All integral types are promoted to XLen width
if (Size < XLen && Ty->isIntegralOrEnumerationType()) {
- return extendType(Ty);
+ return extendType(Ty, CGT.ConvertType(Ty));
}
if (const auto *EIT = Ty->getAs<BitIntType>()) {
if (EIT->getNumBits() < XLen)
- return extendType(Ty);
+ return extendType(Ty, CGT.ConvertType(Ty));
if (EIT->getNumBits() > 128 ||
(!getContext().getTargetInfo().hasInt128Type() &&
EIT->getNumBits() > 64))
@@ -526,12 +526,12 @@ RValue RISCVABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
/*AllowHigherAlign=*/true, Slot);
}
-ABIArgInfo RISCVABIInfo::extendType(QualType Ty) const {
+ABIArgInfo RISCVABIInfo::extendType(QualType Ty, llvm::Type *CoerceTy) const {
int TySize = getContext().getTypeSize(Ty);
// RV64 ABI requires unsigned 32 bit integers to be sign extended.
if (XLen == 64 && Ty->isUnsignedIntegerOrEnumerationType() && TySize == 32)
- return ABIArgInfo::getSignExtend(Ty);
- return ABIArgInfo::getExtend(Ty);
+ return ABIArgInfo::getSignExtend(Ty, CoerceTy);
+ return ABIArgInfo::getExtend(Ty, CoerceTy);
}
namespace {
diff --git a/clang/lib/CodeGen/Targets/X86.cpp b/clang/lib/CodeGen/Targets/X86.cpp
index f71872e77fe823..7e051e475f9d08 100644
--- a/clang/lib/CodeGen/Targets/X86.cpp
+++ b/clang/lib/CodeGen/Targets/X86.cpp
@@ -881,8 +881,8 @@ ABIArgInfo X86_32ABIInfo::classifyArgumentType(QualType Ty, CCState &State,
if (isPromotableIntegerTypeForABI(Ty)) {
if (InReg)
- return ABIArgInfo::getExtendInReg(Ty);
- return ABIArgInfo::getExtend(Ty);
+ return ABIArgInfo::getExtendInReg(Ty, CGT.ConvertType(Ty));
+ return ABIArgInfo::getExtend(Ty, CGT.ConvertType(Ty));
}
if (const auto *EIT = Ty->getAs<BitIntType>()) {
@@ -2756,7 +2756,7 @@ X86_64ABIInfo::classifyArgumentType(QualType Ty, unsigned freeIntRegs,
if (Ty->isIntegralOrEnumerationType() &&
isPromotableIntegerTypeForABI(Ty))
- return ABIArgInfo::getExtend(Ty);
+ return ABIArgInfo::getExtend(Ty, CGT.ConvertType(Ty));
}
break;
diff --git a/clang/test/CodeGen/PowerPC/transparent_union.c b/clang/test/CodeGen/PowerPC/transparent_union.c
deleted file mode 100644
index 968a385c0ee45f..00000000000000
--- a/clang/test/CodeGen/PowerPC/transparent_union.c
+++ /dev/null
@@ -1,67 +0,0 @@
-// RUN: %clang_cc1 -triple powerpc64le-unknown-linux -O2 -target-cpu pwr7 \
-// RUN: -emit-llvm -fshort-enums %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-64
-// RUN: %clang_cc1 -triple powerpc64-unknown-linux -O2 -target-cpu pwr7 \
-// RUN: -emit-llvm -fshort-enums %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-64
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -O2 -target-cpu pwr7 \
-// RUN: -emit-llvm -fshort-enums %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-32
-// RUN: %clang_cc1 -triple powerpc64-unknown-aix -O2 -target-cpu pwr7 \
-// RUN: -emit-llvm -fshort-enums %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-64
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -O2 -target-cpu pwr7 \
-// RUN: -emit-llvm -fshort-enums %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-32
-
-typedef union tu_c {
- signed char a;
- signed char b;
-} tu_c_t __attribute__((transparent_union));
-
-typedef union tu_s {
- short a;
-} tu_s_t __attribute__((transparent_union));
-
-typedef union tu_us {
- unsigned short a;
-} tu_us_t __attribute__((transparent_union));
-
-typedef union tu_l {
- long a;
-} tu_l_t __attribute__((transparent_union));
-
-// CHECK-LABEL: define{{.*}} void @ftest0(
-// CHECK-SAME: i8 noundef signext [[UC_COERCE:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
-// CHECK-NEXT: [[ENTRY:.*:]]
-// CHECK-NEXT: ret void
-void ftest0(tu_c_t uc) { }
-
-// CHECK-LABEL: define{{.*}} void @ftest1(
-// CHECK-SAME: i16 noundef signext [[UC_COERCE:%.*]]) local_unnamed_addr #[[ATTR0]] {
-// CHECK-NEXT: [[ENTRY:.*:]]
-// CHECK-NEXT: ret void
-void ftest1(tu_s_t uc) { }
-
-// CHECK-LABEL: define{{.*}} void @ftest2(
-// CHECK-SAME: i16 noundef zeroext [[UC_COERCE:%.*]]) local_unnamed_addr #[[ATTR0]] {
-// CHECK-NEXT: [[ENTRY:.*:]]
-// CHECK-NEXT: ret void
-void ftest2(tu_us_t uc) { }
-
-// CHECK-64-LABEL: define{{.*}} void @ftest3(
-// CHECK-64-SAME: i64 [[UC_COERCE:%.*]]) local_unnamed_addr #[[ATTR0]] {
-// CHECK-64-NEXT: [[ENTRY:.*:]]
-// CHECK-64-NEXT: ret void
-//
-// CHECK-32-LABEL: define{{.*}} void @ftest3(
-// CHECK-32-SAME: i32 [[UC_COERCE:%.*]]) local_unnamed_addr #[[ATTR0]] {
-// CHECK-32-NEXT: [[ENTRY:.*:]]
-// CHECK-32-NEXT: ret void
-void ftest3(tu_l_t uc) { }
-
-typedef union etest {
- enum flag {red, yellow, blue} fl;
- enum weekend {sun, sat} b;
-} etest_t __attribute__((transparent_union));
-
-// CHECK-LABEL: define{{.*}} void @ftest4(
-// CHECK-SAME: i8 noundef zeroext [[A_COERCE:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
-// CHECK-NEXT: [[ENTRY:.*:]]
-// CHECK-NEXT: ret void
-void ftest4(etest_t a) {}
diff --git a/clang/test/CodeGen/transparent-union-type.c b/clang/test/CodeGen/transparent-union-type.c
new file mode 100644
index 00000000000000..9972dd2131d1b5
--- /dev/null
+++ b/clang/test/CodeGen/transparent-union-type.c
@@ -0,0 +1,81 @@
+// RUN: %clang_cc1 -triple powerpc64le-linux -O2 -target-cpu pwr7 -emit-llvm \
+// RUN: -fshort-enums %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-64
+// RUN: %clang_cc1 -triple powerpc64-linux -O2 -target-cpu pwr7 -emit-llvm \
+// RUN: -fshort-enums %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-64
+// RUN: %clang_cc1 -triple powerpc-linux -O2 -target-cpu pwr7 -emit-llvm \
+// RUN: -fshort-enums %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-32
+// RUN: %clang_cc1 -triple powerpc64-aix -O2 -target-cpu pwr7 -emit-llvm \
+// RUN: -fshort-enums %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-64
+// RUN: %clang_cc1 -triple powerpc-aix -O2 -target-cpu pwr7 -emit-llvm \
+// RUN: -fshort-enums %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-32
+// RUN: %clang_cc1 -triple riscv64-linux -O2 -emit-llvm -fshort-enums \
+// RUN: %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-64
+// RUN: %clang_cc1 -triple riscv32-linux -O2 -emit-llvm -fshort-enums \
+// RUN: %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-32
+// RUN: %clang_cc1 -triple i386-linux -O2 -emit-llvm -fshort-enums \
+// RUN: %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-32
+// RUN: %clang_cc1 -triple x86_64-linux -O2 -emit-llvm -fshort-enums \
+// RUN: %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-64
+// RUN: %clang_cc1 -triple armv7-linux -O2 -emit-llvm -fshort-enums \
+// RUN: %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-32
+// RUN: %clang_cc1 -triple arm64 -target-abi darwinpcs -O2 -emit-llvm \
+// RUN: -fshort-enums %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-64
+// RUN: %clang_cc1 -triple aarch64 -target-abi darwinpcs -O2 -emit-llvm \
+// RUN: -fshort-enums %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-64
+
+typedef union tu_c {
+ signed char a;
+ signed char b;
+} tu_c_t __attribute__((transparent_union));
+
+typedef union tu_s {
+ short a;
+} tu_s_t __attribute__((transparent_union));
+
+typedef union tu_us {
+ unsigned short a;
+} tu_us_t __attribute__((transparent_union));
+
+typedef union tu_l {
+ long a;
+} tu_l_t __attribute__((transparent_union));
+
+// CHECK-LABEL: define{{.*}} void @ftest0(
+// CHECK-SAME: i8 noundef signext [[UC_COERCE:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT: [[ENTRY:.*:]]
+// CHECK-NEXT: ret void
+void ftest0(tu_c_t uc) { }
+
+// CHECK-LABEL: define{{.*}} void @ftest1(
+// CHECK-SAME: i16 noundef signext [[UC_COERCE:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT: [[ENTRY:.*:]]
+// CHECK-NEXT: ret void
+void ftest1(tu_s_t uc) { }
+
+// CHECK-LABEL: define{{.*}} void @ftest2(
+// CHECK-SAME: i16 noundef zeroext [[UC_COERCE:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT: [[ENTRY:.*:]]
+// CHECK-NEXT: ret void
+void ftest2(tu_us_t uc) { }
+
+// CHECK-64-LABEL: define{{.*}} void @ftest3(
+// CHECK-64-SAME: i64 [[UC_COERCE:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-64-NEXT: [[ENTRY:.*:]]
+// CHECK-64-NEXT: ret void
+//
+// CHECK-32-LABEL: define{{.*}} void @ftest3(
+// CHECK-32-SAME: i32 [[UC_COERCE:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-32-NEXT: [[ENTRY:.*:]]
+// CHECK-32-NEXT: ret void
+void ftest3(tu_l_t uc) { }
+
+typedef union etest {
+ enum flag {red, yellow, blue} fl;
+ enum weekend {sun, sat} b;
+} etest_t __attribute__((transparent_union));
+
+// CHECK-LABEL: define{{.*}} void @ftest4(
+// CHECK-SAME: i8 noundef zeroext [[A_COERCE:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT: [[ENTRY:.*:]]
+// CHECK-NEXT: ret void
+void ftest4(etest_t a) {}
|
Contributor
|
May it make sense to add tests with the argument passed in memory / by reference? |
Contributor
|
I think the issue could be handled a different (more generic) way, by pulling |
Contributor
Author
I am not familiar with the calling convention and ABI to know if this is how it should be done. It sounds like a reasonable improvement in how we can implement this. However I feel like it is out of scope for what I am trying to do right now. Would this be better as a separate PR to change how this is handled? This PR would fix the current codegen bug that is seen. |
Contributor
Sure |
amy-kwan
approved these changes
Sep 3, 2024
Contributor
amy-kwan
left a comment
amy-kwan
left a comment
There was a problem hiding this comment.
LGTM for using CGT.ConvertType() like you did previously. I agree that if we were to do a larger/refactoring patch, that can be separate since this PR fixes the current codegen.
lei137
force-pushed
the
lei/transparent_union_2
branch
from
07207ff to
bc296d9
Compare
Contributor
Author
@s-barannikov Done. |
lei137
force-pushed
the
lei/transparent_union_2
branch
from
ed4d8c5 to
6cc5fff
Compare
Update codegen for func param with transparent_union attr to be that of the first union member. This closes llvm#76773.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update codegen for func param with transparent_union attr to be that of
the first union member.
This is a followup to #101738 to fix non-ppc codegen and closes #76773.