[PowerPC] Fix codegen for transparent_union function params

[lei137]

Update codegen for func param with transparent_union attr to be that of the first union member.

[llvmbot]

@llvm/pr-subscribers-backend-powerpc

@llvm/pr-subscribers-clang-codegen

Author: Lei Huang (lei137)

Changes

Update codegen for func param with transparent_union attr to be that of the first union member.

---

Full diff: https://github.com/llvm/llvm-project/pull/101738.diff

5 Files Affected:

• (modified) clang/lib/CodeGen/ABIInfoImpl.cpp (+7)
• (modified) clang/lib/CodeGen/ABIInfoImpl.h (+4)
• (modified) clang/lib/CodeGen/Targets/PPC.cpp (+20-6)
• (added) clang/test/CodeGen/PowerPC/transparent_union.c (+54)
• (added) llvm/test/CodeGen/PowerPC/transparent_union.ll (+67)

diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp b/clang/lib/CodeGen/ABIInfoImpl.cpp
index 35e8f79ba1bac..d73b7e882fe65 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -143,13 +143,20 @@ bool CodeGen::classifyReturnType(const CGCXXABI &CXXABI, CGFunctionInfo &FI,
 }
 
 QualType CodeGen::useFirstFieldIfTransparentUnion(QualType Ty) {
+  bool IsTransparentUnion;
+  return useFirstFieldIfTransparentUnion(Ty, IsTransparentUnion);
+}
+
+QualType CodeGen::useFirstFieldIfTransparentUnion(QualType Ty, bool &TU) {
   if (const RecordType *UT = Ty->getAsUnionType()) {
     const RecordDecl *UD = UT->getDecl();
     if (UD->hasAttr<TransparentUnionAttr>()) {
       assert(!UD->field_empty() && "sema created an empty transparent union");
+      TU = true;
       return UD->field_begin()->getType();
     }
   }
+  TU = false;
   return Ty;
 }
 
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 2a3ef6b8a6c96..95e48ee49d5a4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -65,6 +65,10 @@ CGCXXABI::RecordArgABI getRecordArgABI(QualType T, CGCXXABI &CXXABI);
 bool classifyReturnType(const CGCXXABI &CXXABI, CGFunctionInfo &FI,
                         const ABIInfo &Info);
 
+// For transparent union types, return the type of the first element.
+// Set reference TU to true if Ty given was a transparent union.
+QualType useFirstFieldIfTransparentUnion(QualType Ty, bool &TU);
+
 /// Pass transparent unions as if they were the type of the first element. Sema
 /// should ensure that all elements of the union have the same "machine type".
 QualType useFirstFieldIfTransparentUnion(QualType Ty);
diff --git a/clang/lib/CodeGen/Targets/PPC.cpp b/clang/lib/CodeGen/Targets/PPC.cpp
index e4155810963eb..d2a3abbe24861 100644
--- a/clang/lib/CodeGen/Targets/PPC.cpp
+++ b/clang/lib/CodeGen/Targets/PPC.cpp
@@ -196,7 +196,8 @@ ABIArgInfo AIXABIInfo::classifyReturnType(QualType RetTy) const {
 }
 
 ABIArgInfo AIXABIInfo::classifyArgumentType(QualType Ty) const {
-  Ty = useFirstFieldIfTransparentUnion(Ty);
+  bool IsTransparentUnion;
+  Ty = useFirstFieldIfTransparentUnion(Ty, IsTransparentUnion);
 
   if (Ty->isAnyComplexType())
     return ABIArgInfo::getDirect();
@@ -217,8 +218,14 @@ ABIArgInfo AIXABIInfo::classifyArgumentType(QualType Ty) const {
                                    /*Realign*/ TyAlign > CCAlign);
   }
 
-  return (isPromotableTypeForABI(Ty) ? ABIArgInfo::getExtend(Ty)
-                                     : ABIArgInfo::getDirect());
+  if (isPromotableTypeForABI(Ty))
+    return (IsTransparentUnion ?
+        ABIArgInfo::getExtend(Ty,
+            llvm::IntegerType::get(getVMContext(),
+                                   getContext().getTypeSize(Ty)))
+        : ABIArgInfo::getExtend(Ty));
+
+  return (ABIArgInfo::getDirect());
 }
 
 CharUnits AIXABIInfo::getParamTypeAlignment(QualType Ty) const {
@@ -822,7 +829,8 @@ bool PPC64_SVR4_ABIInfo::isHomogeneousAggregateSmallEnough(
 
 ABIArgInfo
 PPC64_SVR4_ABIInfo::classifyArgumentType(QualType Ty) const {
-  Ty = useFirstFieldIfTransparentUnion(Ty);
+  bool IsTransparentUnion;
+  Ty = useFirstFieldIfTransparentUnion(Ty, IsTransparentUnion);
 
   if (Ty->isAnyComplexType())
     return ABIArgInfo::getDirect();
@@ -891,8 +899,14 @@ PPC64_SVR4_ABIInfo::classifyArgumentType(QualType Ty) const {
                                    /*Realign=*/TyAlign > ABIAlign);
   }
 
-  return (isPromotableTypeForABI(Ty) ? ABIArgInfo::getExtend(Ty)
-                                     : ABIArgInfo::getDirect());
+  if (isPromotableTypeForABI(Ty))
+    return (IsTransparentUnion ?
+        ABIArgInfo::getExtend(Ty,
+            llvm::IntegerType::get(getVMContext(),
+                                   getContext().getTypeSize(Ty)))
+        : ABIArgInfo::getExtend(Ty));
+
+  return ABIArgInfo::getDirect();
 }
 
 ABIArgInfo
diff --git a/clang/test/CodeGen/PowerPC/transparent_union.c b/clang/test/CodeGen/PowerPC/transparent_union.c
new file mode 100644
index 0000000000000..6c61ce553ba7d
--- /dev/null
+++ b/clang/test/CodeGen/PowerPC/transparent_union.c
@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 -triple powerpc64le-unknown-unknown -O2 -target-cpu pwr7 \
+// RUN:   -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-64
+// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -O2 -target-cpu pwr7 \
+// RUN:   -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-64
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix -O2 -target-cpu pwr7 \
+// RUN:   -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-64
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -O2 -target-cpu pwr7 \
+// RUN:   -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-AIX-32
+
+typedef union tu_c {
+	char a;
+	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-AIX-32-LABEL: define void @ftest3(
+// CHECK-AIX-32-SAME: i32 [[UC_COERCE:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-AIX-32-NEXT:  [[ENTRY:.*:]]
+// CHECK-AIX-32-NEXT:    ret void
+void ftest3(tu_l_t uc) { }
diff --git a/llvm/test/CodeGen/PowerPC/transparent_union.ll b/llvm/test/CodeGen/PowerPC/transparent_union.ll
new file mode 100644
index 0000000000000..d04a010737421
--- /dev/null
+++ b/llvm/test/CodeGen/PowerPC/transparent_union.ll
@@ -0,0 +1,67 @@
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux -mcpu=pwr7 \
+; RUN:   -O2 -o - < %s 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-64
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux -mcpu=pwr7 \
+; RUN:   -O2 -o - < %s 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-64
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-aix -mcpu=pwr7 \
+; RUN:   -O2 -o - < %s 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-64
+; RUN: llc -verify-machineinstrs -mtriple=powerpc-unknown-aix -mcpu=pwr7 \
+; RUN:   -O2 -o - < %s 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-AIX-32
+
+%union.tu_c = type { i8 }
+%union.tu_s = type { i16 }
+%union.tu_us = type { i16 }
+%union.tu_l = type { i64 }
+
+define void @ftest0(i8 noundef zeroext %uc.coerce) {
+; CHECK-LABEL: ftest0:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    stb 3, -1(1)
+; CHECK-NEXT:    blr
+entry:
+  %uc = alloca %union.tu_c, align 1
+  %coerce.dive = getelementptr inbounds %union.tu_c, ptr %uc, i32 0, i32 0
+  store i8 %uc.coerce, ptr %coerce.dive, align 1
+  ret void
+}
+
+define void @ftest1(i16 noundef signext %uc.coerce) {
+; CHECK-LABEL: ftest1:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    sth 3, -2(1)
+; CHECK-NEXT:    blr
+entry:
+  %uc = alloca %union.tu_s, align 2
+  %coerce.dive = getelementptr inbounds %union.tu_s, ptr %uc, i32 0, i32 0
+  store i16 %uc.coerce, ptr %coerce.dive, align 2
+  ret void
+}
+
+define void @ftest2(i16 noundef zeroext %uc.coerce) {
+; CHECK-LABEL: ftest2:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    sth 3, -2(1)
+; CHECK-NEXT:    blr
+entry:
+  %uc = alloca %union.tu_us, align 2
+  %coerce.dive = getelementptr inbounds %union.tu_us, ptr %uc, i32 0, i32 0
+  store i16 %uc.coerce, ptr %coerce.dive, align 2
+  ret void
+}
+
+define void @ftest3(i64 %uc.coerce) {
+; CHECK-64-LABEL: ftest3:
+; CHECK-64:       # %bb.0: # %entry
+; CHECK-64-NEXT:    std 3, -8(1)
+; CHECK-64-NEXT:    blr
+;
+; CHECK-AIX-32-LABEL: ftest3:
+; CHECK-AIX-32:       # %bb.0: # %entry
+; CHECK-AIX-32-NEXT:    stw 4, -4(1)
+; CHECK-AIX-32-NEXT:    stw 3, -8(1)
+; CHECK-AIX-32-NEXT:    blr
+entry:
+  %uc = alloca %union.tu_l, align 8
+  %coerce.dive = getelementptr inbounds %union.tu_l, ptr %uc, i32 0, i32 0
+  store i64 %uc.coerce, ptr %coerce.dive, align 8
+  ret void
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[hubert-reinterpretcast]

Code changes seem missing for --target=powerpc-unknown-linux-gnu.

[efriedma-quic]

transparent_union generally means that the value is passed using the convention of the first member of the union. Why do you need to check for whether the value was originally a transparent union? Shouldn't the resulting convention be the same either way?

[lei137]

transparent_union generally means that the value is passed using the convention of the first member of the union. Why do you need to check for whether the value was originally a transparent union? Shouldn't the resulting convention be the same either way?

That would make sense... but to extend the transparent union type based on it's first member we have call getExtend with the llvm::type that we want to extend to. Otherwise it tries to extend the original union QualType. The static function ABIArgInfo getZeroExtend() that is called sets the ABIArgInfo to be the llvm::type given, otherwise it seem to default to the original if that optional param is nullptr.

[efriedma-quic]

Would it do any harm to just unconditionally compute the type and pass it into getExtend()?

[lei137]

Would it do any harm to just unconditionally compute the type and pass it into getExtend()?

This seem to cause issues for type _Bool, where it changes the function param from i1 noundef zeroext %b1 to i8 noundef zeroext %b1.coerce.

$cat a.c
void fcall(_Bool);

_Bool test_wc_i1(_Bool b1, _Bool b2) {
  _Bool o=b1+b2;
  fcall(o);
  return o;
}

IR changes from:

target datalayout = "E-m:e-Fi64-i64:64-n32:64-S128-v256:256:256-v512:512:512"
target triple = "powerpc64-unknown-linux-gnu"

; Function Attrs: nounwind
define dso_local zeroext i1 @test_wc_i1(i1 noundef zeroext %b1, i1 noundef zeroext %b2) local_unnamed_addr #0 {
entry:
  %0 = or i1 %b1, %b2
  tail call void @fcall(i1 noundef zeroext %0) #3
  ret i1 %0
}

declare void @fcall(i1 noundef zeroext) local_unnamed_addr #1

to:

target datalayout = "E-m:e-Fi64-i64:64-n32:64-S128-v256:256:256-v512:512:512"
target triple = "powerpc64-unknown-linux-gnu"

; Function Attrs: nounwind
define dso_local noundef zeroext i1 @test_wc_i1(i8 noundef zeroext %b1.coerce, i8 noundef zeroext %b2.coerce) local_unnamed_addr #0 {
entry:
  %0 = or i8 %b2.coerce, %b1.coerce
  %1 = and i8 %0, 1
  %tobool = icmp ne i8 %1, 0
  tail call void @fcall(i8 noundef zeroext %1) #3
  ret i1 %tobool
}

declare void @fcall(i8 noundef zeroext) local_unnamed_addr #1

This is probably due to the fact I am getting the coerce type via:

llvm::Type *CoerceTy = llvm::IntegerType::get(getVMContext(),  getContext().getTypeSize(Ty));

Not sure if there is a more general way to do this?

[lei137]

I was thinking it would make the code cleaner if we can do something like this instead:

 // For transparent union types, return the type of the first element.
-// Set TU to true if Ty given was a transparent union and to false otherwise.
+// and set CTy the integer type of the first union element.  CTy defaults to nullptr.
-QualType CodeGen::useFirstFieldIfTransparentUnion(QualType Ty, bool &TU) {
+QualType CodeGen::useFirstFieldIfTransparentUnion(QualType Ty,
+                                                  llvm::Type *CTy) {
   if (const RecordType *UT = Ty->getAsUnionType()) {
     const RecordDecl *UD = UT->getDecl();
     if (UD->hasAttr<TransparentUnionAttr>()) {
       assert(!UD->field_empty() && "sema created an empty transparent union");
-      TU = true;
-      return UD->field_begin()->getType();
+      QualType UTy = UD->field_begin()->getType();
+      *CTy = llvm::IntegerType::get(getVMContext(),
+                                    getContext().getTypeSize(UTy));
+      return UTy;
     }
   }
-  TU = false;
   return Ty;
 }

Then we can just call getExtend(Ty, Cty) by default. But I'm not sure how to get access to getVMContext() and getContex() without passing more required params to this function ...

[lei137]

Seems CGT.ConvertType(Ty) does what was needed. I've simplified the patch to use that instead.
@efriedma-quic FYI, this is more inline with your suggestion. Thank-you!

[amy-kwan]

I'm not an expert here, but I think the change you made makes sense since the Ty is supposed to be the first field.

Also, might be a dumb question, I saw we have 32-bit Linux run lines, but is that still worth testing?

[lei137]

I'm not an expert here, but I think the change you made makes sense since the Ty is supposed to be the first field.

Also, might be a dumb question, I saw we have 32-bit Linux run lines, but is that still worth testing?

Thanks for taking a look @amy-kwan. The 32bit linux run line was added in response to review comment from Hubert: #101738 (comment)

[efriedma-quic]

LGTM