[PATCH] pr13012: Support -fpcc-struct-return for x86-32

Arthur O'Dwyer arthur.j.odwyer at gmail.com
Fri Jun 7 16:42:29 PDT 2013


On Fri, Jun 7, 2013 at 3:22 PM, John McCall <rjmccall at apple.com> wrote:
> On Jun 7, 2013, at 12:07 PM, Arthur O'Dwyer <arthur.j.odwyer at gmail.com> wrote:
>>
>> +VALUE_CODEGENOPT(PassSmallStructsConvention, 2, 0) /// If
>> -fpcc-struct-return (==1) or -freg-struct-return (==2) is specified.
>
> Use ENUM_CODEGENOPT instead of magic numbers, please.

Fixed. I renamed this variable to SmallStructsConvention and the enum to
SmallStructsConventionKind to save a bit of horizontal space.
I removed the "default: assert(false);" case in my helper function, as it was
now triggering a warning from -Wcovered-switch-default.

> "Override the default ABI to encourage small structs to be returned in registers"

I strongly prefer not to use the word "encourage", as that makes it sound like
it's an optional optimization or something. Really it changes the ABI so that
small structs WILL be returned in registers, full stop. See my proposed revised
wording below.

> Please error out if the target is not i386, since you've only added
> support for the switch on that target.

Fixed, I believe.
clang: error: unsupported option '-fpcc-struct-return' for target
'x86_64-apple-darwin12.3.0'

Thanks for taking on this review, John!

-Arthur


Index: include/clang/Frontend/CodeGenOptions.h
===================================================================
--- include/clang/Frontend/CodeGenOptions.h (revision 183584)
+++ include/clang/Frontend/CodeGenOptions.h (working copy)
@@ -71,6 +71,12 @@
     FPC_Fast        // Aggressively fuse FP ops (E.g. FMA).
   };

+  enum SmallStructsConventionKind {
+    SSC_Default,  // No special option was passed.
+    SSC_OnStack,  // Small structs on the stack (-fpcc-struct-return).
+    SSC_InRegs    // Small structs in registers (-freg-struct-return).
+  };
+
   /// The code model to use (-mcmodel).
   std::string CodeModel;

Index: include/clang/Frontend/CodeGenOptions.def
===================================================================
--- include/clang/Frontend/CodeGenOptions.def (revision 183584)
+++ include/clang/Frontend/CodeGenOptions.def (working copy)
@@ -85,6 +85,10 @@
                                         ///< enabled.
 VALUE_CODEGENOPT(OptimizationLevel, 3, 0) ///< The -O[0-4] option specified.
 VALUE_CODEGENOPT(OptimizeSize, 2, 0) ///< If -Os (==1) or -Oz (==2)
is specified.
+
+  /// If -fpcc-struct-return or -freg-struct-return is specified.
+ENUM_CODEGENOPT(SmallStructsConvention, SmallStructsConventionKind,
2, SSC_Default)
+
 CODEGENOPT(RelaxAll          , 1, 0) ///< Relax all machine code instructions.
 CODEGENOPT(RelaxedAliasing   , 1, 0) ///< Set when
-fno-strict-aliasing is enabled.
 CODEGENOPT(StructPathTBAA    , 1, 0) ///< Whether or not to use
struct-path TBAA.
@@ -143,4 +147,3 @@
 #undef CODEGENOPT
 #undef ENUM_CODEGENOPT
 #undef VALUE_CODEGENOPT
-
Index: include/clang/Driver/Options.td
===================================================================
--- include/clang/Driver/Options.td (revision 183584)
+++ include/clang/Driver/Options.td (working copy)
@@ -659,6 +659,8 @@
   HelpText<"Specify the default maximum struct packing alignment">;
 def fpascal_strings : Flag<["-"], "fpascal-strings">, Group<f_Group>,
Flags<[CC1Option]>,
   HelpText<"Recognize and construct Pascal-style string literals">;
+def fpcc_struct_return : Flag<["-"], "fpcc-struct-return">,
Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Override the default ABI to return all structs on the stack">;
 def fpch_preprocess : Flag<["-"], "fpch-preprocess">, Group<f_Group>;
 def fpic : Flag<["-"], "fpic">, Group<f_Group>;
 def fno_pic : Flag<["-"], "fno-pic">, Group<f_Group>;
@@ -668,6 +670,8 @@
 def fprofile_generate : Flag<["-"], "fprofile-generate">, Group<f_Group>;
 def framework : Separate<["-"], "framework">, Flags<[LinkerInput]>;
 def frandom_seed_EQ : Joined<["-"], "frandom-seed=">,
Group<clang_ignored_f_Group>;
+def freg_struct_return : Flag<["-"], "freg-struct-return">,
Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Override the default ABI to return small structs in registers">;
 def frtti : Flag<["-"], "frtti">, Group<f_Group>;
 def fsched_interblock : Flag<["-"], "fsched-interblock">,
Group<clang_ignored_f_Group>;
 def fshort_enums : Flag<["-"], "fshort-enums">, Group<f_Group>,
Flags<[CC1Option]>,
Index: lib/Frontend/CompilerInvocation.cpp
===================================================================
--- lib/Frontend/CompilerInvocation.cpp (revision 183584)
+++ lib/Frontend/CompilerInvocation.cpp (working copy)
@@ -473,6 +473,15 @@
       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)) {
+      Opts.setSmallStructsConvention(CodeGenOptions::SSC_OnStack);
+    } else {
+      assert(A->getOption().matches(OPT_freg_struct_return));
+      Opts.setSmallStructsConvention(CodeGenOptions::SSC_InRegs);
+    }
+  }
+
   return Success;
 }

Index: lib/Driver/Tools.cpp
===================================================================
--- lib/Driver/Tools.cpp (revision 183584)
+++ lib/Driver/Tools.cpp (working copy)
@@ -2160,6 +2160,18 @@
     CmdArgs.push_back(A->getValue());
   }

+  if (Arg *A = Args.getLastArg(options::OPT_fpcc_struct_return,
options::OPT_freg_struct_return)) {
+    if (getToolChain().getTriple().getArch() != llvm::Triple::x86) {
+      D.Diag(diag::err_drv_unsupported_opt_for_target)
+        << A->getSpelling() << getToolChain().getTriple().str();
+    } else if (A->getOption().matches(options::OPT_fpcc_struct_return)) {
+      CmdArgs.push_back("-fpcc-struct-return");
+    } else {
+      assert(A->getOption().matches(options::OPT_freg_struct_return));
+      CmdArgs.push_back("-freg-struct-return");
+    }
+  }
+
   if (Args.hasFlag(options::OPT_mrtd, options::OPT_mno_rtd, false))
     CmdArgs.push_back("-mrtd");

Index: lib/CodeGen/TargetInfo.cpp
===================================================================
--- lib/CodeGen/TargetInfo.cpp (revision 183584)
+++ lib/CodeGen/TargetInfo.cpp (working copy)
@@ -575,6 +575,9 @@
       bool d, bool p, bool w, unsigned r)
     :TargetCodeGenInfo(new X86_32ABIInfo(CGT, d, p, w, r)) {}

+  static bool isStructReturnInRegABI(
+      const llvm::Triple &Triple, const CodeGenOptions &Opts);
+
   void SetTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
                            CodeGen::CodeGenModule &CGM) const;

@@ -4410,7 +4413,36 @@
   return ResAddr;
 }

+bool X86_32TargetCodeGenInfo::isStructReturnInRegABI(
+    const llvm::Triple &Triple, const CodeGenOptions &Opts) {
+  assert(Triple.getArch() == llvm::Triple::x86);

+  switch (Opts.getSmallStructsConvention()) {
+  case CodeGenOptions::SSC_Default:
+    break;
+  case CodeGenOptions::SSC_OnStack:  // -fpcc-struct-return
+    return false;
+  case CodeGenOptions::SSC_InRegs:  // -freg-struct-return
+    return true;
+  }
+
+  if (Triple.isOSDarwin())
+    return true;
+
+  switch (Triple.getOS()) {
+  case llvm::Triple::Cygwin:
+  case llvm::Triple::MinGW32:
+  case llvm::Triple::AuroraUX:
+  case llvm::Triple::DragonFly:
+  case llvm::Triple::FreeBSD:
+  case llvm::Triple::OpenBSD:
+  case llvm::Triple::Bitrig:
+    return true;
+  default:
+    return false;
+  }
+}
+
 ABIArgInfo SystemZABIInfo::classifyReturnType(QualType RetTy) const {
   if (RetTy->isVoidType())
     return ABIArgInfo::getIgnore();
@@ -5472,31 +5504,20 @@
     return *(TheTargetCodeGenInfo = new TCETargetCodeGenInfo(Types));

   case llvm::Triple::x86: {
-    if (Triple.isOSDarwin())
-      return *(TheTargetCodeGenInfo =
-               new X86_32TargetCodeGenInfo(Types, true, true, false,
-                                           CodeGenOpts.NumRegisterParameters));
+    bool IsDarwinVectorABI = Triple.isOSDarwin();
+    bool IsSmallStructInRegABI =
+        X86_32TargetCodeGenInfo::isStructReturnInRegABI(Triple, CodeGenOpts);
+    bool IsWin32FloatStructABI = (Triple.getOS() == llvm::Triple::Win32);

-    switch (Triple.getOS()) {
-    case llvm::Triple::Cygwin:
-    case llvm::Triple::MinGW32:
-    case llvm::Triple::AuroraUX:
-    case llvm::Triple::DragonFly:
-    case llvm::Triple::FreeBSD:
-    case llvm::Triple::OpenBSD:
-    case llvm::Triple::Bitrig:
+    if (Triple.getOS() == llvm::Triple::Win32) {
       return *(TheTargetCodeGenInfo =
-               new X86_32TargetCodeGenInfo(Types, false, true, false,
-                                           CodeGenOpts.NumRegisterParameters));
-
-    case llvm::Triple::Win32:
-      return *(TheTargetCodeGenInfo =
                new WinX86_32TargetCodeGenInfo(Types,

CodeGenOpts.NumRegisterParameters));
-
-    default:
+    } else {
       return *(TheTargetCodeGenInfo =
-               new X86_32TargetCodeGenInfo(Types, false, false, false,
+               new X86_32TargetCodeGenInfo(Types,
+                                           IsDarwinVectorABI,
IsSmallStructInRegABI,
+                                           IsWin32FloatStructABI,
                                            CodeGenOpts.NumRegisterParameters));
     }
   }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fpcc-struct-return-v3.diff
Type: application/octet-stream
Size: 7975 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130607/c2a87ae4/attachment.obj>


More information about the cfe-commits mailing list