[clang][PowerPC] Add flag to enable compatibility with GNU for complex arguments #77732

Long5hot · 2024-01-11T06:32:15Z

Fixes : https://github.com/llvm/llvm-project/issues/56023

https://godbolt.org/z/1bsW1sKMs

newFlag : -fcomplex-ppc-gnu-abi

GNU uses GPRs for complex parameters and return values storing for PowerPC-32bit,
which can be enabled which above flag.
Intent of this patch is to make clang compatible with GNU libraries of complex.

Following up with this patch : https://reviews.llvm.org/D146942

llvmbot · 2024-01-11T06:32:42Z

@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Kishan Parmar (Long5hot)

Changes

Fixes : https://github.com/llvm/llvm-project/issues/56023

https://godbolt.org/z/1bsW1sKMs

newFlag : -fcomplex-ppc-gnu-abi

GNU uses GPRs for complex parameters and return values storing for PowerPC-32bit,
which can be enabled which above flag.
Intent of this patch is to make clang compatible with GNU libraries of complex.

Patch is 23.05 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/77732.diff

9 Files Affected:

(modified) clang/include/clang/Basic/CodeGenOptions.def (+2)
(modified) clang/include/clang/Basic/CodeGenOptions.h (+7)
(modified) clang/include/clang/Driver/Options.td (+4)
(modified) clang/lib/CodeGen/CodeGenModule.cpp (+4-2)
(modified) clang/lib/CodeGen/TargetInfo.h (+2-1)
(modified) clang/lib/CodeGen/Targets/PPC.cpp (+98-12)
(modified) clang/lib/Driver/ToolChains/Clang.cpp (+9)
(modified) clang/lib/Frontend/CompilerInvocation.cpp (+8)
(added) clang/test/CodeGen/PowerPC/ppc32-complex-gnu-abi.c (+132)

diff --git a/clang/include/clang/Basic/CodeGenOptions.def b/clang/include/clang/Basic/CodeGenOptions.def
index 2c4fb6745bc172..beeefae15c63f8 100644
--- a/clang/include/clang/Basic/CodeGenOptions.def
+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -213,6 +213,8 @@ CODEGENOPT(MCDCCoverage , 1, 0) ///< Enable MC/DC code coverage criteria.
 
   /// If -fpcc-struct-return or -freg-struct-return is specified.
 ENUM_CODEGENOPT(StructReturnConvention, StructReturnConventionKind, 2, SRCK_Default)
+  /// If -fcomplex-ppc-gnu-abi for ppc32
+ENUM_CODEGENOPT(ComplexInRegABI, ComplexArgumentConventionKind, 2, CMPLX_Default)
 
 CODEGENOPT(RelaxAll          , 1, 0) ///< Relax all machine code instructions.
 CODEGENOPT(RelaxedAliasing   , 1, 0) ///< Set when -fno-strict-aliasing is enabled.
diff --git a/clang/include/clang/Basic/CodeGenOptions.h b/clang/include/clang/Basic/CodeGenOptions.h
index 6952b48e898a81..8abca0e3dda334 100644
--- a/clang/include/clang/Basic/CodeGenOptions.h
+++ b/clang/include/clang/Basic/CodeGenOptions.h
@@ -78,6 +78,13 @@ class CodeGenOptions : public CodeGenOptionsBase {
     SRCK_InRegs    // Small structs in registers (-freg-struct-return).
   };
 
+  enum ComplexArgumentConventionKind {
+    CMPLX_Default,
+    CMPLX_OnStack,
+    CMPLX_OnGPR, // if ppc32 -fcomplex-ppc-gnu-abi
+    CMPLX_OnFPR
+  };
+
   enum ProfileInstrKind {
     ProfileNone,       // Profile instrumentation is turned off.
     ProfileClangInstr, // Clang instrumentation to generate execution counts
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 19becba4a5ad83..251bcb8c49782f 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2540,6 +2540,10 @@ def ffp_contract : Joined<["-"], "ffp-contract=">, Group<f_Group>,
   HelpText<"Form fused FP ops (e.g. FMAs)">,
   Values<"fast,on,off,fast-honor-pragmas">;
 
+def fcomplex_ppc_gnu_abi : Flag<["-"], "fcomplex-ppc-gnu-abi">, Group<f_Group>, Visibility<[ClangOption, CC1Option]>,
+  DocBrief<"Follow the GNU ABI, store Complex values in GPR instead of stack for PowerPC-32">,
+  HelpText<"Store Complex values in GPR instead of stack for PowerPC-32">;
+
 defm strict_float_cast_overflow : BoolFOption<"strict-float-cast-overflow",
   CodeGenOpts<"StrictFloatCastOverflow">, DefaultTrue,
   NegFlag<SetFalse, [], [ClangOption, CC1Option],
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 4fd32337cccc09..003848984d5f7a 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -184,11 +184,13 @@ createTargetCodeGenInfo(CodeGenModule &CGM) {
 
     bool IsSoftFloat =
         CodeGenOpts.FloatABI == "soft" || Target.hasFeature("spe");
-    return createPPC32TargetCodeGenInfo(CGM, IsSoftFloat);
+    unsigned RLen = Target.getPointerWidth(LangAS::Default);
+    return createPPC32TargetCodeGenInfo(CGM, IsSoftFloat, RLen);
   }
   case llvm::Triple::ppcle: {
     bool IsSoftFloat = CodeGenOpts.FloatABI == "soft";
-    return createPPC32TargetCodeGenInfo(CGM, IsSoftFloat);
+    unsigned RLen = Target.getPointerWidth(LangAS::Default);
+    return createPPC32TargetCodeGenInfo(CGM, IsSoftFloat, RLen);
   }
   case llvm::Triple::ppc64:
     if (Triple.isOSAIX())
diff --git a/clang/lib/CodeGen/TargetInfo.h b/clang/lib/CodeGen/TargetInfo.h
index 0c0781a2d5ab9d..89ee2965f6f2e1 100644
--- a/clang/lib/CodeGen/TargetInfo.h
+++ b/clang/lib/CodeGen/TargetInfo.h
@@ -486,7 +486,8 @@ std::unique_ptr<TargetCodeGenInfo>
 createAIXTargetCodeGenInfo(CodeGenModule &CGM, bool Is64Bit);
 
 std::unique_ptr<TargetCodeGenInfo>
-createPPC32TargetCodeGenInfo(CodeGenModule &CGM, bool SoftFloatABI);
+createPPC32TargetCodeGenInfo(CodeGenModule &CGM, bool SoftFloatABI,
+                             unsigned RLen);
 
 std::unique_ptr<TargetCodeGenInfo>
 createPPC64TargetCodeGenInfo(CodeGenModule &CGM);
diff --git a/clang/lib/CodeGen/Targets/PPC.cpp b/clang/lib/CodeGen/Targets/PPC.cpp
index 40dddde508c177..f4885a927ab0ba 100644
--- a/clang/lib/CodeGen/Targets/PPC.cpp
+++ b/clang/lib/CodeGen/Targets/PPC.cpp
@@ -271,22 +271,33 @@ namespace {
 class PPC32_SVR4_ABIInfo : public DefaultABIInfo {
   bool IsSoftFloatABI;
   bool IsRetSmallStructInRegABI;
+  bool isComplexInRegABI;
+  // Size of GPR in bits
+  unsigned RLen;
+  static const int NumArgGPRs = 8;
 
   CharUnits getParamTypeAlignment(QualType Ty) const;
+  ABIArgInfo handleComplex(QualType Ty, uint64_t &TypeSize) const;
 
 public:
   PPC32_SVR4_ABIInfo(CodeGen::CodeGenTypes &CGT, bool SoftFloatABI,
-                     bool RetSmallStructInRegABI)
+                     bool RetSmallStructInRegABI, unsigned RLen,
+                     bool ComplexInRegABI)
       : DefaultABIInfo(CGT), IsSoftFloatABI(SoftFloatABI),
-        IsRetSmallStructInRegABI(RetSmallStructInRegABI) {}
+        IsRetSmallStructInRegABI(RetSmallStructInRegABI),
+        isComplexInRegABI(ComplexInRegABI), RLen(RLen) {}
 
   ABIArgInfo classifyReturnType(QualType RetTy) const;
+  ABIArgInfo classifyArgumentType(QualType Ty, int &ArgGPRsLeft) const;
 
   void computeInfo(CGFunctionInfo &FI) const override {
+
+    int ArgGPRsLeft = NumArgGPRs;
+
     if (!getCXXABI().classifyReturnType(FI))
       FI.getReturnInfo() = classifyReturnType(FI.getReturnType());
     for (auto &I : FI.arguments())
-      I.info = classifyArgumentType(I.type);
+      I.info = classifyArgumentType(I.type, ArgGPRsLeft);
   }
 
   Address EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
@@ -296,9 +307,11 @@ class PPC32_SVR4_ABIInfo : public DefaultABIInfo {
 class PPC32TargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   PPC32TargetCodeGenInfo(CodeGenTypes &CGT, bool SoftFloatABI,
-                         bool RetSmallStructInRegABI)
+                         bool RetSmallStructInRegABI, unsigned RLen,
+                         bool ComplexInRegABI)
       : TargetCodeGenInfo(std::make_unique<PPC32_SVR4_ABIInfo>(
-            CGT, SoftFloatABI, RetSmallStructInRegABI)) {}
+            CGT, SoftFloatABI, RetSmallStructInRegABI, RLen, ComplexInRegABI)) {
+  }
 
   static bool isStructReturnInRegABI(const llvm::Triple &Triple,
                                      const CodeGenOptions &Opts);
@@ -337,12 +350,77 @@ CharUnits PPC32_SVR4_ABIInfo::getParamTypeAlignment(QualType Ty) const {
   return CharUnits::fromQuantity(4);
 }
 
+ABIArgInfo PPC32_SVR4_ABIInfo::handleComplex(QualType Ty,
+                                             uint64_t &TypeSize) const {
+
+  assert(Ty->isAnyComplexType());
+  llvm::Type *ElemTy;
+  unsigned SizeRegs;
+
+  if (TypeSize == 64) {
+    ElemTy = llvm::Type::getInt64Ty(getVMContext());
+    SizeRegs = 1;
+  } else {
+    ElemTy = llvm::Type::getInt32Ty(getVMContext());
+    SizeRegs = (TypeSize + 31) / 32;
+  }
+  return ABIArgInfo::getDirect(llvm::ArrayType::get(ElemTy, SizeRegs));
+}
+
+ABIArgInfo PPC32_SVR4_ABIInfo::classifyArgumentType(QualType Ty,
+                                                    int &ArgGPRsLeft) const {
+
+  assert(ArgGPRsLeft <= NumArgGPRs && "Arg GPR tracking underflow");
+  Ty = useFirstFieldIfTransparentUnion(Ty);
+
+  ASTContext &Context = getContext();
+
+  uint64_t TypeSize = Context.getTypeSize(Ty);
+
+  if (isComplexInRegABI && Ty->isAnyComplexType() &&
+      TypeSize <= RLen * ArgGPRsLeft) {
+    ArgGPRsLeft -= TypeSize / RLen;
+    return handleComplex(Ty, TypeSize);
+  }
+
+  if (isAggregateTypeForABI(Ty)) {
+    if (ArgGPRsLeft)
+      ArgGPRsLeft -= 1;
+    // Records with non-trivial destructors/copy-constructors should not be
+    // passed by value.
+    if (CGCXXABI::RecordArgABI RAA = getRecordArgABI(Ty, getCXXABI())) {
+      return getNaturalAlignIndirect(Ty, RAA == CGCXXABI::RAA_DirectInMemory);
+    }
+    return getNaturalAlignIndirect(Ty);
+  }
+
+  if (!Ty->isFloatingType()) {
+    if (TypeSize > RLen && TypeSize <= 2 * RLen)
+      ArgGPRsLeft -= 2;
+    else
+      ArgGPRsLeft--;
+  }
+
+  // Treat an enum type as its underlying type.
+  if (const EnumType *EnumTy = Ty->getAs<EnumType>())
+    Ty = EnumTy->getDecl()->getIntegerType();
+
+  if (const auto *EIT = Ty->getAs<BitIntType>())
+    if (EIT->getNumBits() >
+        Context.getTypeSize(Context.getTargetInfo().hasInt128Type()
+                                ? Context.Int128Ty
+                                : Context.LongLongTy))
+      return getNaturalAlignIndirect(Ty);
+
+  return (isPromotableIntegerTypeForABI(Ty) ? ABIArgInfo::getExtend(Ty)
+                                            : ABIArgInfo::getDirect());
+}
+
 ABIArgInfo PPC32_SVR4_ABIInfo::classifyReturnType(QualType RetTy) const {
-  uint64_t Size;
+  uint64_t Size = getContext().getTypeSize(RetTy);
 
   // -msvr4-struct-return puts small aggregates in GPR3 and GPR4.
-  if (isAggregateTypeForABI(RetTy) && IsRetSmallStructInRegABI &&
-      (Size = getContext().getTypeSize(RetTy)) <= 64) {
+  if (isAggregateTypeForABI(RetTy) && IsRetSmallStructInRegABI && Size <= 64) {
     // System V ABI (1995), page 3-22, specified:
     // > A structure or union whose size is less than or equal to 8 bytes
     // > shall be returned in r3 and r4, as if it were first stored in the
@@ -361,6 +439,9 @@ ABIArgInfo PPC32_SVR4_ABIInfo::classifyReturnType(QualType RetTy) const {
       return ABIArgInfo::getDirect(CoerceTy);
     }
   }
+  if(isComplexInRegABI && RetTy->isAnyComplexType()) {
+    return handleComplex(RetTy, Size);
+  }
 
   return DefaultABIInfo::classifyReturnType(RetTy);
 }
@@ -372,11 +453,12 @@ Address PPC32_SVR4_ABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAList,
   if (getTarget().getTriple().isOSDarwin()) {
     auto TI = getContext().getTypeInfoInChars(Ty);
     TI.Align = getParamTypeAlignment(Ty);
+    int ArgGPRs = NumArgGPRs;
 
     CharUnits SlotSize = CharUnits::fromQuantity(4);
     return emitVoidPtrVAArg(CGF, VAList, Ty,
-                            classifyArgumentType(Ty).isIndirect(), TI, SlotSize,
-                            /*AllowHigherAlign=*/true);
+                            classifyArgumentType(Ty, ArgGPRs).isIndirect(), TI,
+                            SlotSize, /*AllowHigherAlign=*/true);
   }
 
   const unsigned OverflowLimit = 8;
@@ -974,11 +1056,15 @@ CodeGen::createAIXTargetCodeGenInfo(CodeGenModule &CGM, bool Is64Bit) {
 }
 
 std::unique_ptr<TargetCodeGenInfo>
-CodeGen::createPPC32TargetCodeGenInfo(CodeGenModule &CGM, bool SoftFloatABI) {
+CodeGen::createPPC32TargetCodeGenInfo(CodeGenModule &CGM, bool SoftFloatABI,
+                                      unsigned RLen) {
   bool RetSmallStructInRegABI = PPC32TargetCodeGenInfo::isStructReturnInRegABI(
       CGM.getTriple(), CGM.getCodeGenOpts());
+  bool isComplexInRegABI = (CGM.getCodeGenOpts().getComplexInRegABI() ==
+                            CodeGenOptions::CMPLX_OnGPR);
   return std::make_unique<PPC32TargetCodeGenInfo>(CGM.getTypes(), SoftFloatABI,
-                                                  RetSmallStructInRegABI);
+                                                  RetSmallStructInRegABI, RLen,
+                                                  isComplexInRegABI);
 }
 
 std::unique_ptr<TargetCodeGenInfo>
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 2d8ef841d4f6be..47ad15c100ef45 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -5454,6 +5454,15 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
     }
   }
 
+  if (Arg *A = Args.getLastArg(options::OPT_fcomplex_ppc_gnu_abi)) {
+    if (!TC.getTriple().isPPC32() || !TC.getTriple().isOSBinFormatELF()) {
+      D.Diag(diag::err_drv_unsupported_opt_for_target)
+          << A->getSpelling() << RawTriple.str();
+    } else {
+      CmdArgs.push_back("-fcomplex-ppc-gnu-abi");
+    }
+  }
+
   if (Args.hasFlag(options::OPT_mrtd, options::OPT_mno_rtd, false)) {
     if (Triple.getArch() == llvm::Triple::m68k)
       CmdArgs.push_back("-fdefault-calling-conv=rtdcall");
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 11f3f2c2d6425c..5c1f7da805db87 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -1640,6 +1640,10 @@ void CompilerInvocationBase::GenerateCodeGenArgs(const CodeGenOptions &Opts,
     GenerateArg(Consumer, Opt);
   }
 
+  if (T.isPPC32() && Opts.ComplexInRegABI == CodeGenOptions::CMPLX_OnGPR) {
+    GenerateArg(Consumer, OPT_fcomplex_ppc_gnu_abi);
+  }
+
   if (Opts.EnableAIXExtendedAltivecABI)
     GenerateArg(Consumer, OPT_mabi_EQ_vec_extabi);
 
@@ -2034,6 +2038,10 @@ bool CompilerInvocation::ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args,
     }
   }
 
+  if (Args.getLastArg(OPT_fcomplex_ppc_gnu_abi)) {
+    Opts.setComplexInRegABI(CodeGenOptions::CMPLX_OnGPR);
+  }
+
   if (Arg *A = Args.getLastArg(OPT_mxcoff_roptr)) {
     if (!T.isOSAIX())
       Diags.Report(diag::err_drv_unsupported_opt_for_target)
diff --git a/clang/test/CodeGen/PowerPC/ppc32-complex-gnu-abi.c b/clang/test/CodeGen/PowerPC/ppc32-complex-gnu-abi.c
new file mode 100644
index 00000000000000..b0df1dc836ba81
--- /dev/null
+++ b/clang/test/CodeGen/PowerPC/ppc32-complex-gnu-abi.c
@@ -0,0 +1,132 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 2
+
+// RUN: %clang_cc1 -triple powerpc-unknown-linux-gnu -target-cpu pwr8 \
+// RUN:   -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK-DEF
+// RUN: %clang_cc1 -triple powerpc-unknown-linux-gnu -target-cpu pwr8 -fcomplex-ppc-gnu-abi \
+// RUN:   -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK-GNU
+
+// CHECK-DEF-LABEL: define dso_local void @foo1
+// CHECK-DEF-SAME: (ptr noalias sret({ float, float }) align 4 [[AGG_RESULT:%.*]], ptr noundef byval({ float, float }) align 4 [[X:%.*]]) #[[ATTR0:[0-9]+]] {
+// CHECK-DEF-NEXT:  entry:
+// CHECK-DEF-NEXT:    [[X_REALP:%.*]] = getelementptr inbounds { float, float }, ptr [[X]], i32 0, i32 0
+// CHECK-DEF-NEXT:    [[X_REAL:%.*]] = load float, ptr [[X_REALP]], align 4
+// CHECK-DEF-NEXT:    [[X_IMAGP:%.*]] = getelementptr inbounds { float, float }, ptr [[X]], i32 0, i32 1
+// CHECK-DEF-NEXT:    [[X_IMAG:%.*]] = load float, ptr [[X_IMAGP]], align 4
+// CHECK-DEF-NEXT:    [[AGG_RESULT_REALP:%.*]] = getelementptr inbounds { float, float }, ptr [[AGG_RESULT]], i32 0, i32 0
+// CHECK-DEF-NEXT:    [[AGG_RESULT_IMAGP:%.*]] = getelementptr inbounds { float, float }, ptr [[AGG_RESULT]], i32 0, i32 1
+// CHECK-DEF-NEXT:    store float [[X_REAL]], ptr [[AGG_RESULT_REALP]], align 4
+// CHECK-DEF-NEXT:    store float [[X_IMAG]], ptr [[AGG_RESULT_IMAGP]], align 4
+// CHECK-DEF-NEXT:    [[AGG_RESULT_REALP1:%.*]] = getelementptr inbounds { float, float }, ptr [[AGG_RESULT]], i32 0, i32 0
+// CHECK-DEF-NEXT:    [[AGG_RESULT_REAL:%.*]] = load float, ptr [[AGG_RESULT_REALP1]], align 4
+// CHECK-DEF-NEXT:    [[AGG_RESULT_IMAGP2:%.*]] = getelementptr inbounds { float, float }, ptr [[AGG_RESULT]], i32 0, i32 1
+// CHECK-DEF-NEXT:    [[AGG_RESULT_IMAG:%.*]] = load float, ptr [[AGG_RESULT_IMAGP2]], align 4
+// CHECK-DEF-NEXT:    [[AGG_RESULT_REALP3:%.*]] = getelementptr inbounds { float, float }, ptr [[AGG_RESULT]], i32 0, i32 0
+// CHECK-DEF-NEXT:    [[AGG_RESULT_IMAGP4:%.*]] = getelementptr inbounds { float, float }, ptr [[AGG_RESULT]], i32 0, i32 1
+// CHECK-DEF-NEXT:    store float [[AGG_RESULT_REAL]], ptr [[AGG_RESULT_REALP3]], align 4
+// CHECK-DEF-NEXT:    store float [[AGG_RESULT_IMAG]], ptr [[AGG_RESULT_IMAGP4]], align 4
+// CHECK-DEF-NEXT:    ret void
+//
+// CHECK-GNU-LABEL: define dso_local [1 x i64] @foo1
+// CHECK-GNU-SAME: ([1 x i64] noundef [[X_COERCE:%.*]]) #[[ATTR0:[0-9]+]] {
+// CHECK-GNU-NEXT:  entry:
+// CHECK-GNU-NEXT:    [[RETVAL:%.*]] = alloca { float, float }, align 4
+// CHECK-GNU-NEXT:    [[X:%.*]] = alloca { float, float }, align 4
+// CHECK-GNU-NEXT:    store [1 x i64] [[X_COERCE]], ptr [[X]], align 4
+// CHECK-GNU-NEXT:    [[X_REALP:%.*]] = getelementptr inbounds { float, float }, ptr [[X]], i32 0, i32 0
+// CHECK-GNU-NEXT:    [[X_REAL:%.*]] = load float, ptr [[X_REALP]], align 4
+// CHECK-GNU-NEXT:    [[X_IMAGP:%.*]] = getelementptr inbounds { float, float }, ptr [[X]], i32 0, i32 1
+// CHECK-GNU-NEXT:    [[X_IMAG:%.*]] = load float, ptr [[X_IMAGP]], align 4
+// CHECK-GNU-NEXT:    [[RETVAL_REALP:%.*]] = getelementptr inbounds { float, float }, ptr [[RETVAL]], i32 0, i32 0
+// CHECK-GNU-NEXT:    [[RETVAL_IMAGP:%.*]] = getelementptr inbounds { float, float }, ptr [[RETVAL]], i32 0, i32 1
+// CHECK-GNU-NEXT:    store float [[X_REAL]], ptr [[RETVAL_REALP]], align 4
+// CHECK-GNU-NEXT:    store float [[X_IMAG]], ptr [[RETVAL_IMAGP]], align 4
+// CHECK-GNU-NEXT:    [[TMP0:%.*]] = load [1 x i64], ptr [[RETVAL]], align 4
+// CHECK-GNU-NEXT:    ret [1 x i64] [[TMP0]]
+//
+_Complex float foo1(_Complex float x) {
+  return x;
+}
+
+// CHECK-DEF-LABEL: define dso_local void @foo2
+// CHECK-DEF-SAME: (ptr noalias sret({ double, double }) align 8 [[AGG_RESULT:%.*]], ptr noundef byval({ double, double }) align 8 [[X:%.*]]) #[[ATTR0]] {
+// CHECK-DEF-NEXT:  entry:
+// CHECK-DEF-NEXT:    [[X_REALP:%.*]] = getelementptr inbounds { double, double }, ptr [[X]], i32 0, i32 0
+// CHECK-DEF-NEXT:    [[X_REAL:%.*]] = load double, ptr [[X_REALP]], align 8
+// CHECK-DEF-NEXT:    [[X_IMAGP:%.*]] = getelementptr inbounds { double, double }, ptr [[X]], i32 0, i32 1
+// CHECK-DEF-NEXT:    [[X_IMAG:%.*]] = load double, ptr [[X_IMAGP]], align 8
+// CHECK-DEF-NEXT:    [[AGG_RESULT_REALP:%.*]] = getelementptr inbounds { double, double }, ptr [[AGG_RESULT]], i32 0, i32 0
+// CHECK-DEF-NEXT:    [[AGG_RESULT_IMAGP:%.*]] = getelementptr inbounds { double, double }, ptr [[AGG_RESULT]], i32 0, i32 1
+// CHECK-DEF-NEXT:    store double [[X_REAL]], ptr [[AGG_RESULT_REALP]], align 8
+// CHECK-DEF-NEXT:    store double [[X_IMAG]], ptr [[AGG_RESULT_IMAGP]], align 8
+// CHECK-DEF-NEXT:    [[AGG_RESULT_REALP1:%.*]] = getelementptr inbounds { double, double }, ptr [[AGG_RESULT]], i32 0, i32 0
+// CHECK-DEF-NEXT:    [[AGG_RESULT_REAL:%.*]] = load double, ptr [[AGG_RESULT_REALP1]], align 8
+// CHECK-DEF-NEXT:    [[AGG_RESULT_IMAGP2:%.*]] = getelementptr inbounds { double, double }, ptr [[AGG_RESULT]], i32 0, i32 1
+// CHECK-DEF-NEXT:    [[AGG_RESULT_IMAG:%.*]] = load double, ptr [[AGG_RESULT_IMAGP2]], align 8
+// CHECK-DEF-NEXT:    [[AGG_RESULT_REALP3:%.*]] = getelementptr inbounds { double, double }, ptr [[AGG_RESULT]], i32 0, i32 0
+// CHECK-DEF-NEXT:    [[AGG_RESULT_IMAGP4:%.*]] = getelementptr inbounds { double, double }, ptr [[AGG_RESULT]], i32 0, i32 1
+// CHECK-DEF-NEXT:    store double [[AGG_RESULT_REAL]], ptr [[AGG_RESULT_REALP3]], align 8
+// CHECK-DEF-NEXT:    store double [[AGG_RESULT_IMAG]], ptr [[AGG_RESULT_IMAGP4]], align 8
+// CHECK-DEF-NEXT:    ret void
+//
+// CHECK-GNU-LABEL: define dso_local [4 x i32] @foo2
+// CHECK-GNU-SAME: ([4 x i32] noundef [[X_COERCE:%.*]]) #[[ATTR0]] {
+// CHECK-GNU-NEXT:  entry:
+// CHECK-GNU-NEXT:    [[RETVAL:%.*]] = alloca { double, double }, align 8
+// CHECK-GNU-NEXT:    [[X:%.*]] = alloca { double, double }, align 8
+// CHECK-GNU-NEXT:    store [4 x i32] [[X_COERCE]], ptr [[X]], align 8
+// CHECK-GNU-NEXT:    [[X_REALP:%.*]] = getelementptr inbounds { double, double }, ptr [[X]], i32 0, i32 0
+// CHECK-GNU-NEXT:    [[X_REAL:%.*]] = load double, ptr [[X_REALP]], align 8
+// CHECK-GNU-NEXT:    [[X_IMAGP:%.*]] = getelementptr inbounds { double, double }, ptr [[X]], i32 0, i32 1
+// CHECK-GNU-NEXT:    [[X_IMAG:%.*]] = load double, ptr [[X_IMAGP]], align 8
+// CHECK-GNU-NEXT:    [[RETVAL_REALP:%.*]] = getelementptr inbounds { double, double }, ptr [[RETVAL]], i32 0, i32 0
+// CHECK-GNU-NEXT:    [[RETVAL_IMAGP:%.*]] = getelementptr inbounds { double, double }, ptr [[RETVAL]], i32 0, i32 1
+// CHECK-GNU-NEXT:    store double [[X_REAL]], ptr [[RETVAL_REALP]], align 8
+// CHECK-GNU-NEXT:    store double [[X_IMAG]], ptr [[RETVAL_IMAGP]], align 8
+// CHECK-GNU-NEXT:    [[TMP0:%.*]] = load [4 x i32], ptr [[RETVAL]], align 8
+// CHECK-GNU-NEXT:    ret [4 x i32] [[TMP0]]
+//
+_Complex double foo2(_Complex double x) {
+  return x;
+}
+
+// CHECK-DEF-LABEL: define dso_local void @foo3
+// CHECK-DEF-SAME: (ptr noalias sret({ ppc_fp128, ppc_fp128 }) align 16 [[AGG_RESULT:%.*]], ptr noundef byval({ ppc_fp128, ppc_fp128 }) align 16 [[X:%.*]]) #[[ATTR0]] {
+// CHECK-DEF-NEXT:  entry:
+// CHECK-DEF-NEXT:    [[X_REALP:%.*]] = getelementp...
[truncated]

github-actions · 2024-01-11T06:34:33Z

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

Long5hot · 2024-01-25T08:19:00Z

ping! @chmeeedalf @nemanjai

chmeeedalf · 2024-02-03T21:36:14Z

ping! @chmeeedalf @nemanjai

I know nothing about the complex ABI, so all I can say is the code looks okay from a structural point, can't say anything to the logic.

nemanjai · 2024-01-11T09:12:17Z

clang/include/clang/Basic/CodeGenOptions.h

+  enum ComplexArgumentConventionKind {
+    CMPLX_Default,
+    CMPLX_OnStack,
+    CMPLX_OnGPR, // if ppc32 -fcomplex-ppc-gnu-abi


Nit: can we use In[GF]PR in the names as "in registers"/"on stack" are in common usage.

nemanjai · 2024-01-11T09:13:50Z

clang/include/clang/Driver/Options.td

@@ -2540,6 +2540,10 @@ def ffp_contract : Joined<["-"], "ffp-contract=">, Group<f_Group>,
  HelpText<"Form fused FP ops (e.g. FMAs)">,
  Values<"fast,on,off,fast-honor-pragmas">;

+def fcomplex_ppc_gnu_abi : Flag<["-"], "fcomplex-ppc-gnu-abi">, Group<f_Group>, Visibility<[ClangOption, CC1Option]>,
+  DocBrief<"Follow the GNU ABI, store Complex values in GPR instead of stack for PowerPC-32">,


Please use "Pass" instead of "Store" as the latter generally refers to storing values in memory and this actually affects parameter passing.

nemanjai · 2024-02-08T19:35:12Z

clang/lib/CodeGen/TargetInfo.h

@@ -486,7 +486,8 @@ std::unique_ptr<TargetCodeGenInfo>
 createAIXTargetCodeGenInfo(CodeGenModule &CGM, bool Is64Bit);

 std::unique_ptr<TargetCodeGenInfo>
-createPPC32TargetCodeGenInfo(CodeGenModule &CGM, bool SoftFloatABI);
+createPPC32TargetCodeGenInfo(CodeGenModule &CGM, bool SoftFloatABI,
+                             unsigned RLen);


Please explain this change. It is not clear to me why we're adding this parameter. Are we adding support for 32-bit targets with 64-bit pointers? If so, please justify. Also, this is something that would belong in a separate patch.

I was not sure about hardcoding Register Width(RLen) "32" for RLen here.. https://github.com/llvm/llvm-project/pull/77732/files#diff-05339beb4a6cf7efdfc537984255dd372dcf1edd2d00eabed25831da4d1d0a9fR276

So i used similiar approach like, how RISC-V does to calculate Register Width here.
PPC : https://github.com/llvm/llvm-project/pull/77732/files#diff-e724febedab9c1a2832bf2056d208ff02ddcb2e6f90b5a653afc9b19ac78a5d7R187
RISCV : https://github.com/llvm/llvm-project/pull/77732/files#diff-e724febedab9c1a2832bf2056d208ff02ddcb2e6f90b5a653afc9b19ac78a5d7L226

nemanjai · 2024-02-08T19:37:51Z

My review is not complete, I just submitted what I have so far so at least we can get started on answering the questions I have so far.

diggerlin · 2024-02-22T14:28:23Z

clang/lib/CodeGen/CodeGenModule.cpp

@@ -184,11 +184,13 @@ createTargetCodeGenInfo(CodeGenModule &CGM) {

    bool IsSoftFloat =
        CodeGenOpts.FloatABI == "soft" || Target.hasFeature("spe");
-    return createPPC32TargetCodeGenInfo(CGM, IsSoftFloat);
+    unsigned RLen = Target.getPointerWidth(LangAS::Default);


the variable is used only once , do not need to introduce a new variable.

diggerlin · 2024-02-22T14:28:30Z

clang/lib/CodeGen/CodeGenModule.cpp

  }
  case llvm::Triple::ppcle: {
    bool IsSoftFloat = CodeGenOpts.FloatABI == "soft";
-    return createPPC32TargetCodeGenInfo(CGM, IsSoftFloat);
+    unsigned RLen = Target.getPointerWidth(LangAS::Default);
+    return createPPC32TargetCodeGenInfo(CGM, IsSoftFloat, RLen);


the variable RLen is used only once , do not need to introduce a new variable.

diggerlin · 2024-02-22T14:56:09Z

clang/lib/CodeGen/Targets/PPC.cpp

@@ -271,22 +271,33 @@ namespace {
 class PPC32_SVR4_ABIInfo : public DefaultABIInfo {
  bool IsSoftFloatABI;
  bool IsRetSmallStructInRegABI;
+  bool isComplexInRegABI;
+  // Size of GPR in bits
+  unsigned RLen;


RLen is too short to understand what R means , suggest to change to RegLen and RegisterLen. and Do we need to add RLen ? since the class is PPC32_SVR4_ABIInfo, Should the Register Len be default to 32 bit ?

diggerlin · 2024-02-22T15:53:13Z

clang/lib/CodeGen/Targets/PPC.cpp

+
+  assert(Ty->isAnyComplexType());
+  llvm::Type *ElemTy;
+  unsigned SizeRegs;


can you add a comment for SizeRegs what is stand for?
should it rename to ElemNum ?

diggerlin · 2024-02-22T16:11:19Z

clang/lib/CodeGen/Targets/PPC.cpp

+ABIArgInfo PPC32_SVR4_ABIInfo::handleComplex(QualType Ty,
+                                             uint64_t &TypeSize) const {
+
+  assert(Ty->isAnyComplexType());


it looks the Ty pass in the function is only used for assert ?

diggerlin · 2024-02-22T16:22:10Z

clang/lib/CodeGen/Targets/PPC.cpp

+    ElemTy = llvm::Type::getInt64Ty(getVMContext());
+    SizeRegs = 1;
+  } else {
+    ElemTy = llvm::Type::getInt32Ty(getVMContext());


I am curiously why not get ElemTy = llvm::Type::getInt64Ty(getVMContext()); instead of ElemTy = llvm::Type::getInt32Ty(getVMContext());

and change following code as

llvm::Type *ElemTy = llvm::Type::getInt64Ty(getVMContext()): unsigned SizeRegs = TypeSize/64;

Reason for using llvm::Type::getInt64Ty(getVMContext()) for floats here was to follow ABI ATR-PASS-COMPLEX-IN-GPRS

complex single-precision float : If gr is even, set gr = gr + 1. Load the lower-addressed word of the
argument into gr and the higher-addressed word into gr + 1, set gr = gr + 2.

complex double-precision float: Load the words of the argument, in memory-address order, into gr, gr + 1,
gr + 2 and gr + 3, set gr = gr + 4.

You can check the previous discussion here. : https://reviews.llvm.org/D146942

llvm::Type *ElemTy = llvm::Type::getInt64Ty(getVMContext());
unsigned SizeRegs = TypeSize/64;

Since Typesize will be 64, in if is it necessary to replace 1 to TypeSize/64?

got it, thanks for explain. can you add your explain as comment ?

diggerlin · 2024-02-22T16:42:38Z

clang/lib/CodeGen/Targets/PPC.cpp

+    if (TypeSize > RLen && TypeSize <= 2 * RLen)
+      ArgGPRsLeft -= 2;
+    else
+      ArgGPRsLeft--;


There is a potential bug here, you decrease the ArgGPRsLeft no mater whether ArgGPRsLeft >0;

and change the code here as

if(!Ty->isFloatingType() && TypeSize <= RLen * ArgGPRsLeft) ArgGPRsLeft -= TypeSize / RLen;

amy-kwan

I think the braces can also be omitted on the conditions within the clang/* files.

amy-kwan · 2024-02-23T16:51:27Z

clang/include/clang/Driver/Options.td

@@ -2540,6 +2540,10 @@ def ffp_contract : Joined<["-"], "ffp-contract=">, Group<f_Group>,
  HelpText<"Form fused FP ops (e.g. FMAs)">,
  Values<"fast,on,off,fast-honor-pragmas">;

+def fcomplex_ppc_gnu_abi : Flag<["-"], "fcomplex-ppc-gnu-abi">, Group<f_Group>, Visibility<[ClangOption, CC1Option]>,
+  DocBrief<"Follow the GNU ABI, store Complex values in GPR instead of stack for PowerPC-32">,


Suggested change

DocBrief<"Follow the GNU ABI, store Complex values in GPR instead of stack for PowerPC-32">,

DocBrief<"Follow the GNU ABI, pass Complex values in GPRs instead of the stack for PowerPC-32">,

amy-kwan · 2024-02-23T16:52:06Z

clang/include/clang/Basic/CodeGenOptions.def

@@ -223,6 +223,8 @@ CODEGENOPT(MCDCCoverage , 1, 0) ///< Enable MC/DC code coverage criteria.

  /// If -fpcc-struct-return or -freg-struct-return is specified.
 ENUM_CODEGENOPT(StructReturnConvention, StructReturnConventionKind, 2, SRCK_Default)
+  /// If -fcomplex-ppc-gnu-abi for ppc32


Suggested change

/// If -fcomplex-ppc-gnu-abi for ppc32

/// If -fcomplex-ppc-gnu-abi is specified on ppc32.

This sentence might read a bit better.

amy-kwan · 2024-02-23T16:54:49Z

clang/lib/CodeGen/Targets/PPC.cpp

+  bool isComplexInRegABI;
+  // Size of GPR in bits


Suggested change

bool isComplexInRegABI;

// Size of GPR in bits

bool IsComplexInRegABI;

// Size of GPR in bits.

End sentence with a period, and capitalize the boolean variable for consistency.

amy-kwan · 2024-02-23T17:20:48Z

clang/lib/CodeGen/Targets/PPC.cpp

+ABIArgInfo PPC32_SVR4_ABIInfo::handleComplex(QualType Ty,
+                                             uint64_t &TypeSize) const {
+
+  assert(Ty->isAnyComplexType());


It would also be good if the assert has a message.
Additionally, for people who do not do assert builds, since Ty is only used for the assert, they may get an unused variable warning.

if only use for the assert, suggest putting assert outside the function,

amy-kwan · 2024-02-23T17:23:00Z

clang/lib/CodeGen/Targets/PPC.cpp

@@ -361,6 +439,9 @@ ABIArgInfo PPC32_SVR4_ABIInfo::classifyReturnType(QualType RetTy) const {
      return ABIArgInfo::getDirect(CoerceTy);
    }
  }
+  if (isComplexInRegABI && RetTy->isAnyComplexType()) {


Braces not needed here.

diggerlin · 2024-02-26T20:17:37Z

clang/lib/CodeGen/Targets/PPC.cpp

+                                : Context.LongLongTy))
+      return getNaturalAlignIndirect(Ty);
+
+  return (isPromotableIntegerTypeForABI(Ty) ? ABIArgInfo::getExtend(Ty)


since most of the code is same as DefaultABIInfo::classifyArgumentType(QualType Ty) , I suggest rewrite the function PPC32_SVR4_ABIInfo::classifyArgumentType as

ABIArgInfo PPC32_SVR4_ABIInfo::classifyArgumentType(QualType Ty, int &ArgGPRsLeft) const { .... special functionality code of the function here ..... DefaultABIInfo::classifyArgumentType(Ty) }

duplication code is not a good idea.

diggerlin · 2024-02-26T20:33:44Z

clang/include/clang/Basic/CodeGenOptions.h

@@ -78,6 +78,13 @@ class CodeGenOptions : public CodeGenOptionsBase {
    SRCK_InRegs    // Small structs in registers (-freg-struct-return).
  };

+  enum ComplexArgumentConventionKind {
+    CMPLX_Default,
+    CMPLX_OnStack,


it looks you define the CMPLX_Default and CMPLX_OnStack , but never use them.

This was for future reference, if any arch wants to use this.

diggerlin · 2024-02-26T20:45:54Z

clang/lib/CodeGen/Targets/PPC.cpp


  void computeInfo(CGFunctionInfo &FI) const override {
+
+    int ArgGPRsLeft = NumArgGPRs;


nit: move the definition to the closest first be used.

Couldn't understand what you mean here..

put int ArgGPRsLeft = NumArgGPRs; in front of

for (auto &I : FI.arguments()) I.info = classifyArgumentType(I.type, ArgGPRsLeft);

diggerlin · 2024-02-26T21:21:27Z

clang/test/CodeGen/PowerPC/ppc32-complex-gnu-abi.c

@@ -0,0 +1,132 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 2
+
+// RUN: %clang_cc1 -triple powerpc-unknown-linux-gnu -target-cpu pwr8 \


do you really need -target-cpu pwr8 since the test case only check generated IR?

diggerlin · 2024-02-26T21:21:49Z

clang/test/CodeGen/PowerPC/ppc32-complex-gnu-abi.c

+
+// RUN: %clang_cc1 -triple powerpc-unknown-linux-gnu -target-cpu pwr8 \
+// RUN:   -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK-DEF
+// RUN: %clang_cc1 -triple powerpc-unknown-linux-gnu -target-cpu pwr8 -fcomplex-ppc-gnu-abi \


diggerlin · 2024-02-27T15:06:55Z

clang/lib/CodeGen/Targets/PPC.cpp

+    return handleComplex(Ty, TypeSize);
+  }
+
+  if (isAggregateTypeForABI(Ty)) {


it look that you only has Complex as parameter in your test case .for example _Complex float foo1(_Complex float x)

I think you need more test case with other parameters to test the functionality of function classifyArgumentType()

diggerlin · 2024-04-05T18:06:27Z

clang/lib/CodeGen/Targets/PPC.cpp

+  Ty = useFirstFieldIfTransparentUnion(Ty);
+
+  if (!(getCodeGenOpts().getComplexInRegABI() == CodeGenOptions::CMPLX_InGPR) ||
+      !ArgGPRsLeft)


if (!(getCodeGenOpts().getComplexInRegABI() == CodeGenOptions::CMPLX_InGPR) || !ArgGPRsLeft)

-->
if (getCodeGenOpts().getComplexInRegABI() != CodeGenOptions::CMPLX_InGPR || !ArgGPRsLeft)

diggerlin · 2024-04-05T18:26:09Z

clang/lib/CodeGen/Targets/PPC.cpp

+      --ArgGPRsLeft; // If gr is even set gr = gr + 1 for TypeSize=64.
+    if (TypeSize <= ArgGPRsLeft * RegLen)
+      ArgGPRsLeft -= TypeSize / RegLen;
+  }


the code line 467-472 is duplicated with the code 452-458 ,I would rewrite the function as

ABIArgInfo PPC32_SVR4_ABIInfo::classifyArgumentType(QualType Ty, int &ArgGPRsLeft) const { Ty = useFirstFieldIfTransparentUnion(Ty); if ( getCodeGenOpts().getComplexInRegABI() != CodeGenOptions::CMPLX_InGPR || !ArgGPRsLeft || (Ty->isFloatingType() && !IsSoftFloatABI) return DefaultABIInfo::classifyArgumentType(Ty); assert(ArgGPRsLeft >= 0 && "Arg GPR must be large or equal than zero"); // Records with non-trivial destructors/copy-constructors should not be // passed by value. if (isAggregateTypeForABI(Ty)) --ArgGPRsLeft; ASTContext &Context = getContext(); uint64_t TypeSize = Context.getTypeSize(Ty); // If gr is even set gr = gr + 1 for TypeSize=64. if (TypeSize == 64 && ArgGPRsLeft % 2 == 1) --ArgGPRsLeft; if (TypeSize <= RegLen * ArgGPRsLeft) { ArgGPRsLeft -= TypeSize / RegLen; return handleComplex(TypeSize); } return DefaultABIInfo::classifyArgumentType(Ty); }

diggerlin · 2024-04-05T18:54:06Z

clang/lib/Driver/ToolChains/Clang.cpp

@@ -5585,6 +5585,15 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
    }
  }

+  if (Arg *A = Args.getLastArg(options::OPT_fcomplex_ppc_gnu_abi)) {
+    if (!TC.getTriple().isPPC32() || !TC.getTriple().isOSBinFormatELF()) {
+      D.Diag(diag::err_drv_unsupported_opt_for_target)


I think we need a test case for this.

diggerlin · 2024-04-05T19:13:11Z

clang/test/CodeGen/PowerPC/ppc32-complex-soft-float-gnu-abi.c

+// CHECK-GNU-SOFT-FLOAT-NEXT:    [[TMP0:%.*]] = load [4 x i32], ptr [[RETVAL]], align 8
+// CHECK-GNU-SOFT-FLOAT-NEXT:    ret [4 x i32] [[TMP0]]
+//
+_Complex double _cdouble(_Complex double x) {


there is not float or double as parameter in the test case , I think we have tested the scenario in the ppc32-complex-gnu-abi.c
, do we still need it ?

diggerlin · 2024-04-05T19:18:01Z

clang/test/CodeGen/PowerPC/ppc32-complex-soft-float-gnu-abi.c

+// CHECK-GNU-SOFT-FLOAT-NEXT:    [[TMP0:%.*]] = load [8 x i32], ptr [[RETVAL]], align 16
+// CHECK-GNU-SOFT-FLOAT-NEXT:    ret [8 x i32] [[TMP0]]
+//
+_Complex long double _cldouble(float f, _Complex long double x) {


do you need the test scenario ?

I think we only need test case which have float or double as parameter and make a _Complex parameter is passed as pointer because option '-msoft-float` is enabled

TestCase is same as above, just replacing integer premitives with floating point, Considering this because for soft float also get's passed in gprs..

diggerlin · 2024-05-03T14:35:27Z

clang/lib/CodeGen/Targets/PPC.cpp

@@ -330,8 +330,12 @@ namespace {
 class PPC32_SVR4_ABIInfo : public DefaultABIInfo {
  bool IsSoftFloatABI;
  bool IsRetSmallStructInRegABI;
+  // Size of GPR in bits.
+  static const unsigned RegLen = 32;


suggest change RegLen to GPRBits as

https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/Targets/PPC.cpp#L615

diggerlin · 2024-05-03T18:10:31Z

clang/test/CodeGen/PowerPC/ppc32-complex-soft-float-gnu-abi.c

+// CHECK-GNU-DEF-NEXT:    ret [8 x i32] [[TMP0]]
+//
+// CHECK-GNU-SOFT-FLOAT-LABEL: define dso_local [8 x i32] @_cldouble
+// CHECK-GNU-SOFT-FLOAT-SAME: (float noundef [[F:%.*]], ptr noundef byval({ ppc_fp128, ppc_fp128 }) align 16 [[X:%.*]]) #[[ATTR0:[0-9]+]] {


by the way, I am curiously that option -msoft-float do not change the float noundef [[F:%.*]] to i32 noundef [[F:%.*]] , it looks there is a different implement between the -msoft-float and '-fcomplex-ppc-gnu-abi' ?

Below assembly's generated with -fcomplex-ppc-gnu-abi -O1 --target=ppc32 -S -msoft-float

"use-soft-float"="true" get's passed in IR in attributes

_Complex double testComplexDouble(float w, _Complex float x, _Complex double z) { return z; } testComplexDouble: # @testComplexDouble .Lfunc_begin2: .cfi_startproc # %bb.0: mr 6, 10 mr 5, 9 mr 4, 8 mr 3, 7 blr .Lfunc_end2: .size testComplexDouble, .Lfunc_end2-.Lfunc_begin2 .cfi_endproc # -- End function

_Complex double checkComplexDoubleOnStack(float x1, _Complex float cf, float x2, _Complex double cd) { return testComplexDouble(x2, cf, cd); } .globl checkComplexDoubleOnStack # -- Begin function checkComplexDoubleOnStack .p2align 2 .type checkComplexDoubleOnStack,@function checkComplexDoubleOnStack: # @checkComplexDoubleOnStack .Lfunc_begin3: .cfi_startproc # %bb.0: lwz 3, 0(8) lwz 4, 4(8) lwz 5, 8(8) lwz 6, 12(8) blr

diggerlin · 2024-05-03T18:15:24Z

clang/test/CodeGen/PowerPC/ppc32-complex-soft-float-gnu-abi.c

+// CHECK-GNU-DEF-NEXT:    store double [[Z_IMAG]], ptr [[RETVAL_IMAGP]], align 8
+// CHECK-GNU-DEF-NEXT:    [[TMP0:%.*]] = load [4 x i32], ptr [[RETVAL]], align 8
+// CHECK-GNU-DEF-NEXT:    ret [4 x i32] [[TMP0]]
+//


nit : using empty line instead using this // line, please do for all of them.

diggerlin · 2024-05-03T18:21:31Z

clang/test/CodeGen/PowerPC/ppc32-complex-soft-float-gnu-abi.c

+// CHECK-GNU-SOFT-FLOAT-NEXT:    [[TMP3:%.*]] = load [4 x i32], ptr [[RETVAL]], align 8
+// CHECK-GNU-SOFT-FLOAT-NEXT:    ret [4 x i32] [[TMP3]]
+//
+_Complex double checkComplexDoubleOnStack(float x1, _Complex float cf, float x2, _Complex double cd)


I think we only the test senario is enough to test the float parameter occups one gpr in the PPC32_SVR4_ABIInfo::classifyArgumentType. We can delete all other scenarios.

diggerlin · 2024-05-03T18:27:44Z

clang/test/Driver/ppc32-fcomplex-ppc-gnu-abi.c

+// RUN:     | FileCheck %s -check-prefix=X86_64
+// X86_64: error: unsupported option '-fcomplex-ppc-gnu-abi' for target 'x86_64'
+
+// RUN: not %clang %s --target=ppc64 -fcomplex-ppc-gnu-abi 2>&1 \


I think two test scenarios is enough ,

if (!TC.getTriple().isPPC32() || !TC.getTriple().isOSBinFormatELF()) {

one for !TC.getTriple().isPPC32() , other for !TC.getTriple().isOSBinFormatELF()

diggerlin · 2024-05-03T18:30:21Z

please do not merge the commit if possible, merging the commit make the review difficult, We want to compare what is changed between the two commit when we review. otherwise we have review the patch from scratch.

diggerlin · 2024-05-06T16:10:39Z

I do not further comment on the PR,

but the PR have different mechanism with "-msoft-float" , the -msoft-float pass "use-soft-float"="true" in the IR, the llc will l put float parameter in the GPR, but the PR implement -fcomplex-ppc-gnu-abi in the frontend ,
There is a potential problem,

_Complex long double _cldouble(float f, _Complex long double x) {
  return x;
}

if compile with option "-msoft-float" and -fcomplex-ppc-gnu-abi to generate the IR, and then delete the use-soft-float"="true" from the IR, and use llc for the IR to generate asm, it will error(it will still put _Complex long double x parameter as pointer), if putting the -fcomplex-ppc-gnu-abi in the IR as attribute will not have has the problem (it will put _Complex long double x as value in GPR).

can we implement -fcomplex-ppc-gnu-abi as "-msoft-float" ? what is your opinion ?

UmeshKalappa0 · 2024-05-07T06:36:32Z

@diggerlin ,

can we implement -fcomplex-ppc-gnu-abi as "-msoft-float" ? what is your opinion ?
that was not the intent here and we need to consultant ABI reference regrading the same ,before we decide on implementation .

diggerlin · 2024-05-07T20:15:32Z

@diggerlin ,

can we implement -fcomplex-ppc-gnu-abi as "-msoft-float" ? what is your opinion ?

that was not the intent here and we need to consultant ABI reference regrading the same ,before we decide on implementation .

I think I do not express comment clearly, My comment should be "Can we implement -fcomplex-ppc-gnu-abi in llc instead of in the Clang frontend similar to how -msoft-float was implemented in llc ?"

I went through the code which implement of the -msoft-float today , I am OK to implement -fcomplex-ppc-gnu-abi as it is.

I do not have further comment on the PR, I approved it, but please wait for several days to see whether there is other comment from other reviewer before you merge.

Long5hot · 2024-05-09T09:14:35Z

ping!! @nemanjai

Long5hot · 2024-06-24T11:07:55Z

Ping!!

Long5hot · 2024-07-09T10:12:25Z

ping @chmeeedalf @daltenty @diggerlin @amy-kwan !!

Long5hot · 2024-08-03T07:17:54Z

ping!

AaronBallman

I'm wondering why the GNU-compatible behavior is not on by default, especially when in GNU mode (e.g., -std=gnu17)?

Also, the changes should come with a release note in clang/docs/ReleaseNotes.rst so users know about the new functionality.

AaronBallman · 2024-08-05T11:45:01Z

clang/lib/Frontend/CompilerInvocation.cpp

+  if (T.isPPC32() && Opts.ComplexInRegABI == CodeGenOptions::CMPLX_InGPR) {
+    GenerateArg(Consumer, OPT_fcomplex_ppc_gnu_abi);
+  }
+


Suggested change

if (T.isPPC32() && Opts.ComplexInRegABI == CodeGenOptions::CMPLX_InGPR) {

GenerateArg(Consumer, OPT_fcomplex_ppc_gnu_abi);

}

if (T.isPPC32() && Opts.ComplexInRegABI == CodeGenOptions::CMPLX_InGPR)

GenerateArg(Consumer, OPT_fcomplex_ppc_gnu_abi);

AaronBallman · 2024-08-05T11:45:16Z

clang/lib/Frontend/CompilerInvocation.cpp

+  if (Args.getLastArg(OPT_fcomplex_ppc_gnu_abi)) {
+    Opts.setComplexInRegABI(CodeGenOptions::CMPLX_InGPR);
+  }


Suggested change

if (Args.getLastArg(OPT_fcomplex_ppc_gnu_abi)) {

Opts.setComplexInRegABI(CodeGenOptions::CMPLX_InGPR);

}

if (Args.getLastArg(OPT_fcomplex_ppc_gnu_abi))

Opts.setComplexInRegABI(CodeGenOptions::CMPLX_InGPR);

AaronBallman · 2024-08-05T11:46:46Z

clang/test/Driver/ppc32-fcomplex-ppc-gnu-abi.c

@@ -0,0 +1,9 @@
+// RUN: %clang -c --target=ppc32 -fcomplex-ppc-gnu-abi %s 2>&1


It'd be nice for this to check -### output to ensure that it's passing along the correct -cc1 option.

MaskRay · 2024-08-05T17:46:26Z

clang/test/CodeGen/PowerPC/ppc32-complex-gnu-abi.c

+// RUN:   -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK-DEF
+// RUN: %clang_cc1 -triple powerpc-unknown-linux-gnu -fcomplex-ppc-gnu-abi \
+// RUN:   -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK-GNU
+


We just use one single blank line as a separator.

Long5hot · 2024-08-08T16:11:09Z

I'm wondering why the GNU-compatible behavior is not on by default, especially when in GNU mode (e.g., -std=gnu17)?

ATR-PASS-COMPLEX-IN-GPRS was missing in clang as shown in ABI doc : https://example61560.wordpress.com/wp-content/uploads/2016/11/powerpc_abi.pdf

Also, the changes should come with a release note in clang/docs/ReleaseNotes.rst so users know about the new functionality.

@AaronBallman, can you guide me, in which section of ReleaseNotes.rst i should add info about new flag?

AaronBallman · 2024-08-09T15:40:26Z

I'm wondering why the GNU-compatible behavior is not on by default, especially when in GNU mode (e.g., -std=gnu17)?

ATR-PASS-COMPLEX-IN-GPRS was missing in clang as shown in ABI doc : https://example61560.wordpress.com/wp-content/uploads/2016/11/powerpc_abi.pdf

I'm sorry but I still don't understand why it's not on by default in GNU mode. If the user is in GNU mode, they presumably are doing so because they want to be GNU-compatible, yes?

Also, the changes should come with a release note in clang/docs/ReleaseNotes.rst so users know about the new functionality.

@AaronBallman, can you guide me, in which section of ReleaseNotes.rst i should add info about new flag?

I'd add it under New Compiler Flags

Long5hot · 2024-08-11T11:31:56Z

We were compiling simple testcase with complex parameters with clang and it was crashing because libraries was built using gcc.

@AaronBallman, Reason for new flag was to enable this for other C standards as well. Currently we use c11 as standard in VxWorks toolchain, which has to work with gcc compiled libraries.
gcc with c11 as well passes complex parameters in GPRs.

AaronBallman · 2024-08-12T12:48:37Z

We were compiling simple testcase with complex parameters with clang and it was crashing because libraries was built using gcc.

@AaronBallman, Reason for new flag was to enable this for other C standards as well. Currently we use c11 as standard in VxWorks toolchain, which has to work with gcc compiled libraries. gcc with c11 as well passes complex parameters in GPRs.

Ah, perhaps I was unclear -- I'm not opposed to the flag existing so users can opt into/out of it as they want. I'm wondering why the flag needs to be specified when compiling with -triple powerpc-unknown-linux-gnu (because that specifies a GNU environment with a powerpc target) or with -triple powerpc -std=gnu<whatever> (because that specifies GNU extensions with a powerpc target). It seems to me that the flag should be implied in those cases, so users shouldn't have to specify it manually, right?

Long5hot · 2024-08-12T15:59:33Z

@AaronBallman, Yes you are right. I will work on it!

…x arguments (llvm#77732) Fixes : llvm#56023 https://godbolt.org/z/1bsW1sKMs newFlag : -fcomplex-ppc-gnu-abi GNU uses GPRs for complex parameters and return values storing for PowerPC-32bit, which can be enabled which above flag. Intent of this patch is to make clang compatible with GNU libraries of complex. Following up with this patch : https://reviews.llvm.org/D146942

Long5hot requested a review from nemanjai January 11, 2024 06:32

Long5hot self-assigned this Jan 11, 2024

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen labels Jan 11, 2024

Long5hot force-pushed the GNU-Complex-Arguments branch from f79b66c to ec05087 Compare January 17, 2024 05:03

Long5hot force-pushed the GNU-Complex-Arguments branch from ec05087 to 6a209fb Compare January 24, 2024 07:46

thesamesam added the backend:PowerPC label Jan 24, 2024

thesamesam mentioned this pull request Jan 24, 2024

PowerPC ABI incompatibility with GNU on complex arguments #56023

Open

Long5hot requested a review from chmeeedalf January 25, 2024 08:19

lei137 requested review from daltenty and diggerlin February 8, 2024 18:11

nemanjai reviewed Feb 8, 2024

View reviewed changes

diggerlin reviewed Feb 22, 2024

View reviewed changes

amy-kwan reviewed Feb 23, 2024

View reviewed changes

diggerlin reviewed Feb 26, 2024

View reviewed changes

diggerlin reviewed Feb 27, 2024

View reviewed changes

Long5hot force-pushed the GNU-Complex-Arguments branch from 6a209fb to a89cd20 Compare March 4, 2024 13:18

Long5hot force-pushed the GNU-Complex-Arguments branch from 81d57b4 to 3c5fcb0 Compare March 28, 2024 11:58

diggerlin reviewed Apr 5, 2024

View reviewed changes

Long5hot force-pushed the GNU-Complex-Arguments branch 3 times, most recently from 4cdfaf0 to cc5fe4a Compare April 28, 2024 08:50

diggerlin reviewed May 3, 2024

View reviewed changes

diggerlin approved these changes May 7, 2024

View reviewed changes

Endilll requested a review from chenzheng1030 August 4, 2024 10:37

AaronBallman requested review from MaskRay and efriedma-quic August 5, 2024 11:42

AaronBallman reviewed Aug 5, 2024

View reviewed changes

MaskRay reviewed Aug 5, 2024

View reviewed changes

Long5hot force-pushed the GNU-Complex-Arguments branch from 473e188 to 2d4ae49 Compare August 8, 2024 10:01

Long5hot force-pushed the GNU-Complex-Arguments branch from 2d4ae49 to f463f28 Compare August 12, 2024 09:00

Long5hot force-pushed the GNU-Complex-Arguments branch from 869a686 to d5c328d Compare February 5, 2025 09:42

	DocBrief<"Follow the GNU ABI, store Complex values in GPR instead of stack for PowerPC-32">,
	DocBrief<"Follow the GNU ABI, pass Complex values in GPRs instead of the stack for PowerPC-32">,

	/// If -fcomplex-ppc-gnu-abi for ppc32
	/// If -fcomplex-ppc-gnu-abi is specified on ppc32.


		void computeInfo(CGFunctionInfo &FI) const override {

		int ArgGPRsLeft = NumArgGPRs;

		@@ -0,0 +1,132 @@
		// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 2

		// RUN: %clang_cc1 -triple powerpc-unknown-linux-gnu -target-cpu pwr8 \

		@@ -0,0 +1,9 @@
		// RUN: %clang -c --target=ppc32 -fcomplex-ppc-gnu-abi %s 2>&1

[clang][PowerPC] Add flag to enable compatibility with GNU for complex arguments #77732

Are you sure you want to change the base?

[clang][PowerPC] Add flag to enable compatibility with GNU for complex arguments #77732

Conversation

Long5hot commented Jan 11, 2024 • edited Loading

llvmbot commented Jan 11, 2024 • edited Loading

github-actions bot commented Jan 11, 2024 • edited Loading

Long5hot commented Jan 25, 2024

chmeeedalf commented Feb 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemanjai commented Feb 8, 2024

Choose a reason for hiding this comment

diggerlin Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diggerlin Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diggerlin Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diggerlin Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

amy-kwan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diggerlin Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diggerlin Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diggerlin Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diggerlin Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diggerlin May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diggerlin commented May 3, 2024 • edited Loading

diggerlin commented May 6, 2024 • edited Loading

UmeshKalappa0 commented May 7, 2024

diggerlin commented May 7, 2024 • edited Loading

Long5hot commented May 9, 2024

Long5hot commented Jun 24, 2024

Long5hot commented Jul 9, 2024

Long5hot commented Aug 3, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Long5hot commented Aug 8, 2024

AaronBallman commented Aug 9, 2024

Long5hot commented Aug 11, 2024

AaronBallman commented Aug 12, 2024

Long5hot commented Jan 11, 2024 •

edited

Loading

llvmbot commented Jan 11, 2024 •

edited

Loading

github-actions bot commented Jan 11, 2024 •

edited

Loading

diggerlin Feb 22, 2024 •

edited

Loading

diggerlin Feb 22, 2024 •

edited

Loading

diggerlin Feb 22, 2024 •

edited

Loading

diggerlin Feb 22, 2024 •

edited

Loading

diggerlin Feb 26, 2024 •

edited

Loading

diggerlin Feb 27, 2024 •

edited

Loading

diggerlin Apr 5, 2024 •

edited

Loading

diggerlin Apr 5, 2024 •

edited

Loading

diggerlin May 3, 2024 •

edited

Loading

diggerlin commented May 3, 2024 •

edited

Loading

diggerlin commented May 6, 2024 •

edited

Loading

diggerlin commented May 7, 2024 •

edited

Loading