[PowerPC] Add clang -msvr4-struct-return for 32-bit ELF

[kernigh]

Summary

This is a patch for https://bugs.llvm.org/show_bug.cgi?id=40736

Beware: This diff passes its own tests, and writes asm that looks correct to me, but I have not yet tried running the code on PowerPC. I intend to backport the diff to llvm/clang 8.0.1 (the version in OpenBSD), and try it on an old and slow PowerPC Macintosh. I upload the diff now, because other people might want to see it.

This is only my 2nd patch on reviews.llvm.org, so I might have filled some fields wrong. I don't have commit access to LLVM.

[PowerPC] Add clang -msvr4-struct-return for 32-bit ELF

Change the default ABI to be compatible with GCC. For 32-bit ELF targets other than Linux, Clang now returns small structs in registers r3/r4. This affects FreeBSD, NetBSD, OpenBSD. There is no change for 32-bit Linux, where Clang continues to return all structs in memory.

Add clang options -maix-struct-return (to return structs in memory) and -msvr4-struct-return (to return structs in registers) to be compatible with gcc. These options are only for PPC32; reject them on PPC64 and other targets. The options are like -fpcc-struct-return and -freg-struct-return for X86_32, and use similar code.

To actually return a struct in registers, coerce it to an integer of the same size. LLVM may optimize the code to remove unnecessary accesses to memory, and will return i32 in r3 or i64 in r3:r4.

Fixes PR#40736

[markmi]

Summary: My preliminary testing looks good.

FreeBSD does not have clang/test/ and clang/doc/ in its contrib/llvm-project/ so I applied just the other patches to my context and rebuilt my FreeBSD head -r356426 based context's clang/clang++ and such.
Using the updated compilers I did buildworld and buildkernel for 32-bit powerpc, installed them, and booted an old PowerMac 2-socket G4 with the result. After that I built the ports related to devel/freebsd-gcc9 at powerpc and lang/gcc9 and installed the packages the build produced. I then used such toolchains to build and run the original program that I found the ABI mismatch with, building without using -maix-struct-return but using FreeBSD's libc++ and such (like before).

The program ran fine.

The overall sequence I used involved buildworld buildkernel for powerpc64 and installing and booting such as well. That was in turn used to build some ports for 32-bit powerpc via devel/poudriere (so via chroot activity). All that worked fine.

It will be some time before all my usual ports have been built for 32-bit powerpc. But my preliminary testing is good news.

[markmi]

I built and installed ports, what built vs. did-not was the same as before applying the update, with the same failure details having nothing to do with this change. I then ran the FreeBSD kyua tests and the result match what I got prior to the update.

These tests are not about gcc/g++ ABI compatibility, but FreeBSD seems to work fine when built based on the change.

(FYI, given the ABI incompatibility, I did not try to self-host getting from one ABI to the other. I instead installed the updated FreeBSD as a separate drive on another system and then put that drive back in its normal system and booted from it.)

I'm not aware of anything around that makes for a good, fairly-general gcc/g++ ABI compatibility test.

[brad]

Now that 10 is out, any chance of getting some movement on this to resolve this ABI issue with 32-bit PowerPC?

[jhibbits]

Code looks fine, and others have tested it. Good to see a reversion of the ABI to expected for GCC compatibility on the BSDs.

[ZarkoCA]

@jhibbits is this patch going to be committed soon? I have a patch (https://reviews.llvm.org/D76360) that I will need to rebase once this is in. Thanks.

[jhibbits]

@ZarkoCA I think someone else should also review this, so added @nemanjai as a potential reviewer. He might have more insight to the code in question. I'd like to see this go in soon, though, so that it gets into 11, and we can use it in FreeBSD.

[nemanjai]

Aside from a couple of minor nits that shouldn't require another review, LGTM.

clang/docs/ClangCommandLineReference.rst

2631

Can you specify that "small" means 8 bytes or smaller?

clang/lib/CodeGen/TargetInfo.cpp

4378

Please add text to the assert to help anyone who ends up tripping it. Perhaps:

assert(Triple.getArch() == llvm::Triple::ppc &&
       "Invalid triple for a 32-bit PowerPC target");

clang/docs/ClangCommandLineReference.rst

 .. option:: -msingle-float
 
 .. option:: -mvirt, -mno-virt
 
 .. option:: -mxgot, -mno-xgot
 
 PowerPC
 -------
+.. option:: -maix-struct-return
+
+Override the default ABI for 32-bit targets to return all structs in memory,
+as in the Power 32-bit ABI for Linux (2011), and on AIX and Darwin.
+
 .. option:: -maltivec, -mno-altivec
 
 .. option:: -mcmpb, -mno-cmpb
 
 .. option:: -mcrbits, -mno-crbits
 
 .. option:: -mcrypto, -mno-crypto
 

 .. option:: -mpower8-vector, -mno-power8-vector
 
 .. option:: -mpower9-vector, -mno-power9-vector
 
 .. option:: -mqpx, -mno-qpx
 
 .. option:: -msecure-plt
 
+.. option:: -msvr4-struct-return
+
+Override the default ABI for 32-bit targets to return small structs in
+registers, as in the System V ABI (1995).
+
 .. option:: -mvsx, -mno-vsx
 
 WebAssembly
 -----------
 .. option:: -mexception-handling, -mno-exception-handling
 
 .. option:: -mnontrapping-fptoint, -mno-nontrapping-fptoint
 

clang/include/clang/Driver/Options.td

 def mfloat128: Flag<["-"], "mfloat128">,
     Group<m_ppc_Features_Group>;
 def mno_float128 : Flag<["-"], "mno-float128">,
     Group<m_ppc_Features_Group>;
 def mlongcall: Flag<["-"], "mlongcall">,
     Group<m_ppc_Features_Group>;
 def mno_longcall : Flag<["-"], "mno-longcall">,
     Group<m_ppc_Features_Group>;
+def maix_struct_return : Flag<["-"], "maix-struct-return">,
+  Group<m_Group>, Flags<[CC1Option]>,
+  HelpText<"Return all structs in memory (PPC32 only)">;
+def msvr4_struct_return : Flag<["-"], "msvr4-struct-return">,
+  Group<m_Group>, Flags<[CC1Option]>,
+  HelpText<"Return small structs in registers (PPC32 only)">;
 
 def mvx : Flag<["-"], "mvx">, Group<m_Group>;
 def mno_vx : Flag<["-"], "mno-vx">, Group<m_Group>;
 
 def fzvector : Flag<["-"], "fzvector">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Enable System z vector language extension">;
 def fno_zvector : Flag<["-"], "fno-zvector">, Group<f_Group>,
   Flags<[CC1Option]>;

clang/lib/CodeGen/TargetInfo.cpp

return emitVoidPtrVAArg(CGF, VAListAddr, Ty, IsIndirect,
                           /*allowHigherAlign*/ false);
 }
 
 // PowerPC-32
 namespace {
 /// PPC32_SVR4_ABIInfo - The 32-bit PowerPC ELF (SVR4) ABI information.
 class PPC32_SVR4_ABIInfo : public DefaultABIInfo {
   bool IsSoftFloatABI;
+  bool IsRetSmallStructInRegABI;
 
   CharUnits getParamTypeAlignment(QualType Ty) const;
 
 public:
-  PPC32_SVR4_ABIInfo(CodeGen::CodeGenTypes &CGT, bool SoftFloatABI)
-      : DefaultABIInfo(CGT), IsSoftFloatABI(SoftFloatABI) {}
+  PPC32_SVR4_ABIInfo(CodeGen::CodeGenTypes &CGT, bool SoftFloatABI,
+                     bool RetSmallStructInRegABI)
+      : DefaultABIInfo(CGT), IsSoftFloatABI(SoftFloatABI),
+        IsRetSmallStructInRegABI(RetSmallStructInRegABI) {}
+
+  ABIArgInfo classifyReturnType(QualType RetTy) const;
+
+  void computeInfo(CGFunctionInfo &FI) const override {
+    if (!getCXXABI().classifyReturnType(FI))
+      FI.getReturnInfo() = classifyReturnType(FI.getReturnType());
+    for (auto &I : FI.arguments())
+      I.info = classifyArgumentType(I.type);
+  }
 
   Address EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
                     QualType Ty) const override;
 };
 
 class PPC32TargetCodeGenInfo : public TargetCodeGenInfo {
 public:
-  PPC32TargetCodeGenInfo(CodeGenTypes &CGT, bool SoftFloatABI)
-      : TargetCodeGenInfo(new PPC32_SVR4_ABIInfo(CGT, SoftFloatABI)) {}
+  PPC32TargetCodeGenInfo(CodeGenTypes &CGT, bool SoftFloatABI,
+                         bool RetSmallStructInRegABI)
+      : TargetCodeGenInfo(new PPC32_SVR4_ABIInfo(CGT, SoftFloatABI,
+                                                 RetSmallStructInRegABI)) {}
+
+  static bool isStructReturnInRegABI(const llvm::Triple &Triple,
+                                     const CodeGenOptions &Opts);
 
   int getDwarfEHStackPointer(CodeGen::CodeGenModule &M) const override {
     // This is recovered from gcc output.
     return 1; // r1 is the dedicated stack pointer
   }
 
   bool initDwarfEHRegSizeTable(CodeGen::CodeGenFunction &CGF,
                                llvm::Value *Address) const override;
if ((EltType->isVectorType() && getContext().getTypeSize(EltType) == 128) ||
       AlignTy = EltType;
   }
 
   if (AlignTy)
     return CharUnits::fromQuantity(AlignTy->isVectorType() ? 16 : 4);
   return CharUnits::fromQuantity(4);
 }
 
+ABIArgInfo PPC32_SVR4_ABIInfo::classifyReturnType(QualType RetTy) const {
+  uint64_t Size;
+
+  // -msvr4-struct-return puts small aggregates in GPR3 and GPR4.
+  if (isAggregateTypeForABI(RetTy) && IsRetSmallStructInRegABI &&
+      (Size = getContext().getTypeSize(RetTy)) <= 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
+    // > 8-byte aligned memory area and then the low addressed word were
+    // > loaded into r3 and the high-addressed word into r4.  Bits beyond
+    // > the last member of the structure or union are not defined.
+    //
+    // GCC for big-endian PPC32 inserts the pad before the first member,
+    // not "beyond the last member" of the struct.  To stay compatible
+    // with GCC, we coerce the struct to an integer of the same size.
+    // LLVM will extend it and return i32 in r3, or i64 in r3:r4.
+    if (Size == 0)
+      return ABIArgInfo::getIgnore();
+    else {
+      llvm::Type *CoerceTy = llvm::Type::getIntNTy(getVMContext(), Size);
+      return ABIArgInfo::getDirect(CoerceTy);
+    }
+  }
+
+  return DefaultABIInfo::classifyReturnType(RetTy);
+}
+
 // TODO: this implementation is now likely redundant with
 // DefaultABIInfo::EmitVAArg.
 Address PPC32_SVR4_ABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAList,
                                       QualType Ty) const {
   if (getTarget().getTriple().isOSDarwin()) {
     auto TI = getContext().getTypeInfoInChars(Ty);
     TI.second = getParamTypeAlignment(Ty);
 
Address PPC32_SVR4_ABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAList,
   if (isIndirect) {
     Result = Address(Builder.CreateLoad(Result, "aggr"),
                      getContext().getTypeAlignInChars(Ty));
   }
 
   return Result;
 }
 
+bool PPC32TargetCodeGenInfo::isStructReturnInRegABI(
+    const llvm::Triple &Triple, const CodeGenOptions &Opts) {
+  assert(Triple.getArch() == llvm::Triple::ppc);
+
+  switch (Opts.getStructReturnConvention()) {
+  case CodeGenOptions::SRCK_Default:
+    break;
+  case CodeGenOptions::SRCK_OnStack: // -maix-struct-return
+    return false;
+  case CodeGenOptions::SRCK_InRegs: // -msvr4-struct-return
+    return true;
+  }
+
+  if (Triple.isOSBinFormatELF() && !Triple.isOSLinux())
+    return true;
+
+  return false;
+}
+
 bool
 PPC32TargetCodeGenInfo::initDwarfEHRegSizeTable(CodeGen::CodeGenFunction &CGF,
                                                 llvm::Value *Address) const {
   // This is calculated from the LLVM and GCC tables and verified
   // against gcc output.  AFAIK all ABIs use the same encoding.
 
   CodeGen::CGBuilderTy &Builder = CGF.Builder;
 
else if (CodeGenOpts.FloatABI == "hard" ||
               (Triple.getEnvironment() == llvm::Triple::GNUEABIHF ||
                Triple.getEnvironment() == llvm::Triple::MuslEABIHF ||
                Triple.getEnvironment() == llvm::Triple::EABIHF)))
       Kind = ARMABIInfo::AAPCS_VFP;
 
     return SetCGInfo(new ARMTargetCodeGenInfo(Types, Kind));
   }
 
-  case llvm::Triple::ppc:
+  case llvm::Triple::ppc: {
+    bool IsSoftFloat =
+        CodeGenOpts.FloatABI == "soft" || getTarget().hasFeature("spe");
+    bool RetSmallStructInRegABI =
+        PPC32TargetCodeGenInfo::isStructReturnInRegABI(Triple, CodeGenOpts);
     return SetCGInfo(
-        new PPC32TargetCodeGenInfo(Types, CodeGenOpts.FloatABI == "soft" ||
-                                   getTarget().hasFeature("spe")));
+        new PPC32TargetCodeGenInfo(Types, IsSoftFloat, RetSmallStructInRegABI));
+  }
   case llvm::Triple::ppc64:
     if (Triple.isOSBinFormatELF()) {
       PPC64_SVR4_ABIInfo::ABIKind Kind = PPC64_SVR4_ABIInfo::ELFv1;
       if (getTarget().getABI() == "elfv2")
         Kind = PPC64_SVR4_ABIInfo::ELFv2;
       bool HasQPX = getTarget().getABI() == "elfv1-qpx";
       bool IsSoftFloat = CodeGenOpts.FloatABI == "soft";
 

clang/lib/Driver/ToolChains/Clang.cpp

if (!Args.hasFlag(options::OPT_fpreserve_as_comments,
                     options::OPT_fno_preserve_as_comments, true))
     CmdArgs.push_back("-fno-preserve-as-comments");
 
   if (Arg *A = Args.getLastArg(options::OPT_mregparm_EQ)) {
     CmdArgs.push_back("-mregparm");
     CmdArgs.push_back(A->getValue());
   }
 
+  if (Arg *A = Args.getLastArg(options::OPT_maix_struct_return,
+                               options::OPT_msvr4_struct_return)) {
+    if (TC.getArch() != llvm::Triple::ppc) {
+      D.Diag(diag::err_drv_unsupported_opt_for_target)
+          << A->getSpelling() << RawTriple.str();
+    } else if (A->getOption().matches(options::OPT_maix_struct_return)) {
+      CmdArgs.push_back("-maix-struct-return");
+    } else {
+      assert(A->getOption().matches(options::OPT_msvr4_struct_return));
+      CmdArgs.push_back("-msvr4-struct-return");
+    }
+  }
+
   if (Arg *A = Args.getLastArg(options::OPT_fpcc_struct_return,
                                options::OPT_freg_struct_return)) {
     if (TC.getArch() != llvm::Triple::x86) {
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << A->getSpelling() << RawTriple.str();
     } else if (A->getOption().matches(options::OPT_fpcc_struct_return)) {
       CmdArgs.push_back("-fpcc-struct-return");
     } else {

clang/lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
 
   if (Arg *A = Args.getLastArg(OPT_fdenormal_fp_math_f32_EQ)) {
     StringRef Val = A->getValue();
     Opts.FP32DenormalMode = llvm::parseDenormalFPAttribute(Val);
     if (Opts.FP32DenormalMode == llvm::DenormalMode::Invalid)
       Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
   }
 
-  if (Arg *A = Args.getLastArg(OPT_fpcc_struct_return, OPT_freg_struct_return)) {
-    if (A->getOption().matches(OPT_fpcc_struct_return)) {
+  // X86_32 has -fppc-struct-return and -freg-struct-return.
+  // PPC32 has -maix-struct-return and -msvr4-struct-return.
+  if (Arg *A =
+          Args.getLastArg(OPT_fpcc_struct_return, OPT_freg_struct_return,
+                          OPT_maix_struct_return, OPT_msvr4_struct_return)) {
+    const Option &O = A->getOption();
+    if (O.matches(OPT_fpcc_struct_return) ||
+        O.matches(OPT_maix_struct_return)) {
       Opts.setStructReturnConvention(CodeGenOptions::SRCK_OnStack);
     } else {
-      assert(A->getOption().matches(OPT_freg_struct_return));
+      assert(O.matches(OPT_freg_struct_return) ||
+             O.matches(OPT_msvr4_struct_return));
       Opts.setStructReturnConvention(CodeGenOptions::SRCK_InRegs);
     }
   }
 
   Opts.DependentLibraries = Args.getAllArgValues(OPT_dependent_lib);
   Opts.LinkerOptions = Args.getAllArgValues(OPT_linker_option);
   bool NeedLocTracking = false;
 

clang/test/CodeGen/ppc32-struct-return.c

+// REQUIRES: powerpc-registered-target
+// RUN: %clang_cc1 -triple powerpc-unknown-freebsd \
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
+// RUN: %clang_cc1 -triple powerpc-unknown-linux \
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
+// RUN: %clang_cc1 -triple powerpc-unknown-linux -maix-struct-return \
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
+// RUN: %clang_cc1 -triple powerpc-unknown-linux -msvr4-struct-return \
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
+// RUN: %clang_cc1 -triple powerpc-unknown-netbsd \
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
+// RUN: %clang_cc1 -triple powerpc-unknown-openbsd \
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
+// RUN: %clang_cc1 -triple powerpc-unknown-openbsd -maix-struct-return \
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
+// RUN: %clang_cc1 -triple powerpc-unknown-openbsd -msvr4-struct-return \
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
+
+typedef struct {
+} Zero;
+typedef struct {
+  char c;
+} One;
+typedef struct {
+  short s;
+} Two;
+typedef struct {
+  char c[3];
+} Three;
+typedef struct {
+  float f;
+} Four; // svr4 to return i32, not float
+typedef struct {
+  char c[5];
+} Five;
+typedef struct {
+  short s[3];
+} Six;
+typedef struct {
+  char c[7];
+} Seven;
+typedef struct {
+  int i;
+  char c;
+} Eight; // padded for alignment
+typedef struct {
+  char c[9];
+} Nine;
+
+// CHECK-AIX-LABEL: define void @ret0(%struct.Zero* noalias sret {{[^,]*}})
+// CHECK-SVR4-LABEL: define void @ret0()
+Zero ret0(void) { return (Zero){}; }
+
+// CHECK-AIX-LABEL: define void @ret1(%struct.One* noalias sret {{[^,]*}})
+// CHECK-SVR4-LABEL: define i8 @ret1()
+One ret1(void) { return (One){'a'}; }
+
+// CHECK-AIX-LABEL: define void @ret2(%struct.Two* noalias sret {{[^,]*}})
+// CHECK-SVR4-LABEL: define i16 @ret2()
+Two ret2(void) { return (Two){123}; }
+
+// CHECK-AIX-LABEL: define void @ret3(%struct.Three* noalias sret {{[^,]*}})
+// CHECK-SVR4-LABEL: define i24 @ret3()
+Three ret3(void) { return (Three){"abc"}; }
+
+// CHECK-AIX-LABEL: define void @ret4(%struct.Four* noalias sret {{[^,]*}})
+// CHECK-SVR4-LABEL: define i32 @ret4()
+Four ret4(void) { return (Four){0.4}; }
+
+// CHECK-AIX-LABEL: define void @ret5(%struct.Five* noalias sret {{[^,]*}})
+// CHECK-SVR4-LABEL: define i40 @ret5()
+Five ret5(void) { return (Five){"abcde"}; }
+
+// CHECK-AIX-LABEL: define void @ret6(%struct.Six* noalias sret {{[^,]*}})
+// CHECK-SVR4-LABEL: define i48 @ret6()
+Six ret6(void) { return (Six){12, 34, 56}; }
+
+// CHECK-AIX-LABEL: define void @ret7(%struct.Seven* noalias sret {{[^,]*}})
+// CHECK-SVR4-LABEL: define i56 @ret7()
+Seven ret7(void) { return (Seven){"abcdefg"}; }
+
+// CHECK-AIX-LABEL: define void @ret8(%struct.Eight* noalias sret {{[^,]*}})
+// CHECK-SVR4-LABEL: define i64 @ret8()
+Eight ret8(void) { return (Eight){123, 'a'}; }
+
+// CHECK-AIX-LABEL: define void @ret9(%struct.Nine* noalias sret {{[^,]*}})
+// CHECK-SVR4-LABEL: define void @ret9(%struct.Nine* noalias sret {{[^,]*}})
+Nine ret9(void) { return (Nine){"abcdefghi"}; }

clang/test/Driver/ppc-unsupported.c

+// REQUIRES: powerpc-registered-target
+// RUN: not %clang -target powerpc64-unknown-freebsd -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-freebsd -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64le-unknown-linux -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64le-unknown-linux -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// CHECK: unsupported option