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

[Eric Christopher]

Binary sounds good. There's already code that handles things like
subtarget-features based on command lines, maybe extend that or in that
same area handle function attributes on a per-target basis?

-eric

On Thu, Apr 30, 2015 at 2:30 PM Akira Hatanaka <ahatanak at gmail.com> wrote:

> On Thu, Apr 30, 2015 at 1:45 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
>
>>
>> > On 2015-Apr-30, at 13:00, Akira Hatanaka <ahatanak at gmail.com> wrote:
>> >
>> > Hi echristo, dexonsmith,
>> >
>> > 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.
>>
>> Binary SGTM.
>>
>> >
>> > 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::cl::ParseCommandLineOptions?
>>
>> IMO, we should create a proper -cc1 option, stop using
>> `-backend-option`, and stop passing anything to
>> `cl::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.
>>
>> So in this example, #2 would behave the same, but #1 would stop
>> enforcing -arm-long-calls (since old.bc doesn't have arm-long-calls
>> encoded, and -mlong-calls only adds attributes to functions in IRGen).
>>
>>   - AFAICT, there wasn't a supported way to pass -arm-long-calls to
>>     LTO until now (via attributes), so we won't regress behaviour
>>     there.
>>   - Outside of LTO, archiving bitcode and later codegen'ing from
>>     clang isn't supported, unless I'm missing something?
>>
>>
> That's correct. I wasn't worried about the first case, but wasn't sure
> about the second case. If we don't have to support it, binary should be
> fine.
>
>
>> IMO we don't need to worry about backwards compatibility for #1, so
>> I think: make it a binary option and stop using the `cl::opt`.
>>
>> >
>> > http://reviews.llvm.org/D9414
>> >
>> > Files:
>> >  include/clang/Frontend/CodeGenOptions.h
>> >  lib/CodeGen/CGCall.cpp
>> >  lib/Frontend/CompilerInvocation.cpp
>> >  test/CodeGen/fn-attr.c
>> >
>> > Index: include/clang/Frontend/CodeGenOptions.h
>> > ===================================================================
>> > --- include/clang/Frontend/CodeGenOptions.h
>> > +++ include/clang/Frontend/CodeGenOptions.h
>> > @@ -151,6 +151,9 @@
>> >   /// 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;
>> >
>> > Index: lib/CodeGen/CGCall.cpp
>> > ===================================================================
>> > --- lib/CodeGen/CGCall.cpp
>> > +++ lib/CodeGen/CGCall.cpp
>> > @@ -1455,6 +1455,9 @@
>> >       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) {
>> > Index: lib/Frontend/CompilerInvocation.cpp
>> > ===================================================================
>> > --- lib/Frontend/CompilerInvocation.cpp
>> > +++ lib/Frontend/CompilerInvocation.cpp
>> > @@ -340,6 +340,14 @@
>> >   }
>> > }
>> >
>> > +static void getFunctionAttributes(CodeGenOptions &Opts) {
>> > +  StringRef Opt = "-arm-long-calls", Key = Opt.drop_front(1);
>> > +
>> > +  if (std::find(Opts.BackendOptions.begin(), Opts.BackendOptions.end(),
>> > +                Opt) != Opts.BackendOptions.end())
>> > +    Opts.FunctionAttributes.push_back(std::make_pair(Key, ""));
>> > +}
>> > +
>> > static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args,
>> InputKind IK,
>> >                              DiagnosticsEngine &Diags,
>> >                              const TargetOptions &TargetOpts) {
>> > @@ -643,6 +651,8 @@
>> >                       Args.getAllArgValues(OPT_fsanitize_recover_EQ),
>> Diags,
>> >                       Opts.SanitizeRecover);
>> >
>> > +  getFunctionAttributes(Opts);
>> > +
>> >   return Success;
>> > }
>> >
>> > Index: test/CodeGen/fn-attr.c
>> > ===================================================================
>> > --- /dev/null
>> > +++ test/CodeGen/fn-attr.c
>> > @@ -0,0 +1,7 @@
>> > +// RUN: %clang_cc1 -triple thumbv7-apple-ios5 -backend-option
>> -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; }
>> >
>> > EMAIL PREFERENCES
>> >  http://reviews.llvm.org/settings/panel/emailpreferences/
>> > <D9414.24762.patch>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150506/627ea9cc/attachment.html>