[ARM] Save option "arm-long-calls" to the IR as a function attribute

[ahatanak]

Summary

This patch is part of the work to make LTO and function multi-versioning work correctly.

Currently, -mlong-calls, which is converted to cc1 option -arm-long-calls, is ignored when building with LTO because the option isn't passed to the linker or libLTO. This patch saves the option in the IR as a function attribute to fix this problem.

The corresponding llvm patch is here:
http://reviews.llvm.org/D9364

There are a few things to discuss:

1. Should "arm-long-calls" be a binary attribute or a tri-state like "unsafe-fp-math" that takes a value "true" or "false"? I made it a binary attribute because it simplifies the backend and frontend without breaking backward compatibility, but might be use cases that I'm unaware of in which this approach wouldn't work.

2. Since we are saving the option in the IR, should we stop passing it as a cc1 backend option and stop passing it to llvm:🆑:ParseCommandLineOptions? It's not needed if this attribute is a tri-state, but is needed if it's a binary to preserve backward compatibility. By "backward compatibility", I mean the following commands should produce the same result before and after this patch is committed:

1. clang -target armv7-apple-ios -static -mlong-calls old.bc -o old
2. clang -target armv7-apple-ios -static old.bc -o old

Here, old.bc is generated by an older version of clang that doesn't save "arm-long-calls" in the IR.

[ahatanak]

Made changes based on feedback comments.

In the new patch, a new cc1 option "function-attribute" is defined and "arm-long-calls" is passed as a function-attribute instead of a backend-option. I think this is better than my previous patch as it doesn't have to check "arm-long-calls", which is a target-specific attribute, in CompilerInvocation.cpp, and it no longer passes the string to cl::ParseCommandLineOptions to toggle the cl::opt option in the backend.

[echristo]

Hi Akira,

I'm not sure how we want to do calling convention type things. I don't know that we want it as a generic attribute on the function or as a subtarget feature. What are your thoughts here?

-eric

[ahatanak]

It seems to me that turning arm-long-calls into a subtarget feature would simplify both clang and llvm.

[ahatanak]

Save "long-calls" as a subtarget feature of ARM instead of a function attribute.

[echristo]

Couple of inline comments, go ahead and commit when you've resolved them.

Thanks!

-eric

lib/Driver/Tools.cpp

773–774

Can you add a plain english comment above this explaining what we're supposed to be checking here? :)

test/CodeGen/arm-long-calls.c

3 ↗

(On Diff #28984)

Test for -mno-long-calls?

[ahatanak]

Add comments to explain how the decision to pass feature "+long-calls" is made.

[ahatanak]

test/CodeGen/arm-long-calls.c

4 ↗

(On Diff #29150)

"+long-calls" (or "-long-calls") is never passed if -mno-calls is specified on the command line. The driver test (arm-long-calls.c) checks that "+long-calls" gets passed based on occurrence of "-mlong-calls" or "-mno-long-calls".

include/clang/Driver/CC1Options.td

 def mlimit_float_precision : Separate<["-"], "mlimit-float-precision">,
   HelpText<"Limit float precision to the given value">;
 def split_stacks : Flag<["-"], "split-stacks">,
   HelpText<"Try to use a split stack if possible.">;
 def mno_zero_initialized_in_bss : Flag<["-"], "mno-zero-initialized-in-bss">,
   HelpText<"Do not put zero initialized data in the BSS">;
 def backend_option : Separate<["-"], "backend-option">,
   HelpText<"Additional arguments to forward to LLVM backend (during code gen)">;
+def function_attribute : Separate<["-"], "function-attribute">,
+  HelpText<"Function attributes to attach to functions (during code gen)">;
 def mregparm : Separate<["-"], "mregparm">,
   HelpText<"Limit the number of registers available for integer arguments">;
 def mrelocation_model : Separate<["-"], "mrelocation-model">,
   HelpText<"The relocation model to use">;
 def munwind_tables : Flag<["-"], "munwind-tables">,
   HelpText<"Generate unwinding tables for all functions">;
 def mconstructor_aliases : Flag<["-"], "mconstructor-aliases">,
   HelpText<"Emit complete constructors and destructors as aliases when possible">;

include/clang/Frontend/CodeGenOptions.h

public:
 
   /// If not an empty string, trap intrinsics are lowered to calls to this
   /// function instead of to trap instructions.
   std::string TrapFuncName;
 
   /// A list of command-line options to forward to the LLVM backend.
   std::vector<std::string> BackendOptions;
 
+  /// A list of function attributes to save to the IR.
+  std::vector<std::pair<std::string, std::string>> FunctionAttributes;
+
   /// A list of dependent libraries.
   std::vector<std::string> DependentLibraries;
 
   /// Name of the profile file to use as output for -fprofile-instr-generate
   std::string InstrProfileOutput;
 
   /// Name of the profile file to use with -fprofile-sample-use.
   std::string SampleProfileFile;

lib/CodeGen/CGCall.cpp

if (CodeGenOpts.EnableSegmentedStacks &&
     FuncAttrs.addAttribute("split-stack");
 
   if (AttrOnCallSite) {
     // Attributes that should go on the call site only.
     if (!CodeGenOpts.SimplifyLibCalls)
       FuncAttrs.addAttribute(llvm::Attribute::NoBuiltin);
   } else {
     // Attributes that should go on the function, but not the call site.
+    for (auto &KV : CodeGenOpts.FunctionAttributes)
+      FuncAttrs.addAttribute(KV.first, KV.second);
+
     if (!CodeGenOpts.DisableFPElim) {
       FuncAttrs.addAttribute("no-frame-pointer-elim", "false");
     } else if (CodeGenOpts.OmitLeafFramePointer) {
       FuncAttrs.addAttribute("no-frame-pointer-elim", "false");
       FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
     } else {
       FuncAttrs.addAttribute("no-frame-pointer-elim", "true");
       FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");

lib/Driver/Tools.cpp

if (Arg *A = Args.getLastArg(options::OPT_mcrc, options::OPT_mnocrc)) {
     else
       Features.push_back("-crc");
   }
 }
 
 void Clang::AddARMTargetArgs(const ArgList &Args,
                              ArgStringList &CmdArgs,
                              bool KernelOrKext) const {
   const Driver &D = getToolChain().getDriver();
   // Get the effective triple, which takes into account the deployment target.
   std::string TripleStr = getToolChain().ComputeEffectiveClangTriple(Args);
   llvm::Triple Triple(TripleStr);
   std::string CPUName = arm::getARMTargetCPU(Args, Triple);
 
   // Select the ABI to use.
   //
   // FIXME: Support -meabi.
   // FIXME: Parts of this are duplicated in the backend, unify this somehow.
if (FloatABI == "soft") {
     assert(FloatABI == "hard" && "Invalid float abi!");
     CmdArgs.push_back("-mfloat-abi");
     CmdArgs.push_back("hard");
   }
 
   // Kernel code has more strict alignment requirements.
   if (KernelOrKext) {
     if (!Triple.isiOS() || Triple.isOSVersionLT(6)) {
-      CmdArgs.push_back("-backend-option");
-      CmdArgs.push_back("-arm-long-calls");
+      CmdArgs.push_back("-function-attribute");
+      CmdArgs.push_back("arm-long-calls");
     }
 
     CmdArgs.push_back("-backend-option");
     CmdArgs.push_back("-arm-strict-align");
 
     // The kext linker doesn't know how to deal with movw/movt.
     CmdArgs.push_back("-backend-option");
     CmdArgs.push_back("-arm-use-movt=0");
if (Arg *A = Args.getLastArg(options::OPT_mrestrict_it,
     CmdArgs.push_back("-arm-restrict-it");
   }
 
   if (TT.getArch() == llvm::Triple::arm ||
       TT.getArch() == llvm::Triple::thumb) {
     if (Arg *A = Args.getLastArg(options::OPT_mlong_calls,
                                  options::OPT_mno_long_calls)) {
       if (A->getOption().matches(options::OPT_mlong_calls)) {
-        CmdArgs.push_back("-backend-option");
-        CmdArgs.push_back("-arm-long-calls");
+        CmdArgs.push_back("-function-attribute");
+        CmdArgs.push_back("arm-long-calls");
       }
     }
   }
 
   // Forward -f options with positive and negative forms; we translate
   // these by hand.
   if (Arg *A = Args.getLastArg(options::OPT_fprofile_sample_use_EQ)) {
     StringRef fname = A->getValue();

lib/Frontend/CompilerInvocation.cpp

#include "clang/Basic/Sanitizers.def"
                           .Default(SanitizerKind::Unknown);
     if (K == SanitizerKind::Unknown)
       Diags.Report(diag::err_drv_invalid_value) << FlagName << Sanitizer;
     else
       S.set(K, true);
   }
 }
 
+static void getFunctionAttributes(CodeGenOptions &Opts,
+                                  const std::vector<std::string> &FnAttrStrs) {
+  for (const auto &S : FnAttrStrs) {
+    std::pair<StringRef, StringRef> KV = StringRef(S).split('=');
+    Opts.FunctionAttributes.push_back(std::make_pair(KV.first, KV.second));
+  }
+}
+
 static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
                              DiagnosticsEngine &Diags,
                              const TargetOptions &TargetOpts) {
   using namespace options;
   bool Success = true;
 
   unsigned OptimizationLevel = getOptimizationLevel(Args, IK, Diags);
   // TODO: This could be done in Driver
static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.RewriteMapFiles = Args.getAllArgValues(OPT_frewrite_map_file);
 
   // Parse -fsanitize-recover= arguments.
   // FIXME: Report unrecoverable sanitizers incorrectly specified here.
   parseSanitizerKinds("-fsanitize-recover=",
                       Args.getAllArgValues(OPT_fsanitize_recover_EQ), Diags,
                       Opts.SanitizeRecover);
 
+  getFunctionAttributes(Opts, Args.getAllArgValues(OPT_function_attribute));
   Opts.CudaGpuBinaryFileNames =
       Args.getAllArgValues(OPT_fcuda_include_gpubinary);
 
   return Success;
 }
 
 static void ParseDependencyOutputArgs(DependencyOutputOptions &Opts,
                                       ArgList &Args) {

test/CodeGen/fn-attr.c

+// RUN: %clang_cc1 -triple thumbv7-apple-ios5 -function-attribute arm-long-calls -emit-llvm -o - %s | FileCheck -check-prefix=LONGCALL %s
+// RUN: %clang_cc1 -triple thumbv7-apple-ios5 -emit-llvm -o - %s | FileCheck -check-prefix=NOLONGCALL %s
+
+// LONGCALL: attributes #0 = { {{.*}} "arm-long-calls
+// NOLONGCALL-NOT: attributes #0 = { {{.*}} "arm-long-calls
+
+int foo1(int a) { return a; }

test/Driver/apple-kext-mkernel.c

 // RUN: %clang -target x86_64-apple-darwin10 \
 // RUN:   -mkernel -### -fsyntax-only %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-X86 < %t %s
 
 // CHECK-X86: "-disable-red-zone"
 // CHECK-X86: "-fno-builtin"
 // CHECK-X86: "-fno-rtti"
 // CHECK-X86: "-fno-common"
 
 // RUN: %clang -target x86_64-apple-darwin10 \
 // RUN:   -arch armv7 -mkernel -mstrict-align -### -fsyntax-only %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-ARM < %t %s
 
-// CHECK-ARM: "-backend-option" "-arm-long-calls"
+// CHECK-ARM: "-function-attribute" "arm-long-calls"
 // CHECK-ARM: "-backend-option" "-arm-strict-align"
 // CHECK-ARM-NOT: "-backend-option" "-arm-strict-align"
 // CHECK-ARM: "-fno-builtin"
 // CHECK-ARM: "-fno-rtti"
 // CHECK-ARM: "-fno-common"
 
 // RUN: %clang -target x86_64-apple-darwin10 \
 // RUN:   -Werror -fno-builtin -fno-exceptions -fno-common -fno-rtti \
 // RUN:   -mkernel -fsyntax-only %s

test/Driver/arm-long-calls.c

 // RUN: %clang -target armv7-eabi -### %s 2>&1 \
 // RUN:    | FileCheck %s -check-prefix CHECK-DEFAULT
 
 // RUN: %clang -target armv7-eabi -### -mlong-calls %s 2>&1 \
 // RUN:    | FileCheck %s -check-prefix CHECK-LONG-CALLS
 
 // RUN: %clang -target armv7-eabi -### -mlong-calls -mno-long-calls %s 2>&1 \
 // RUN:    | FileCheck %s -check-prefix CHECK-NO-LONG-CALLS
 
-// CHECK-DEFAULT-NOT: "-backend-option" "-arm-long-calls"
+// CHECK-DEFAULT-NOT: "-function-attribute" "arm-long-calls"
 
-// CHECK-LONG-CALLS: "-backend-option" "-arm-long-calls"
+// CHECK-LONG-CALLS: "-function-attribute" "arm-long-calls"
 
-// CHECK-NO-LONG-CALLS-NOT: "-backend-option" "-arm-long-calls"
+// CHECK-NO-LONG-CALLS-NOT: "-function-attribute" "arm-long-calls"