<div dir="ltr">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?<br><div><br></div><div>-eric</div></div><br><div class="gmail_quote">On Thu, Apr 30, 2015 at 2:30 PM Akira Hatanaka <<a href="mailto:ahatanak@gmail.com">ahatanak@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Apr 30, 2015 at 1:45 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><br>
> On 2015-Apr-30, at 13:00, Akira Hatanaka <<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>> wrote:<br>
><br>
> Hi echristo, dexonsmith,<br>
><br>
> This patch is part of the work to make LTO and function multi-versioning work correctly.<br>
><br>
> 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.<br>
><br>
> The corresponding llvm patch is here:<br>
> <a href="http://reviews.llvm.org/D9364" target="_blank">http://reviews.llvm.org/D9364</a><br>
><br>
> There are a few things to discuss:<br>
><br>
> 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.<br>
<br>
</span>Binary SGTM.<br>
<span><br>
><br>
> 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?<br>
<br>
</span>IMO, we should create a proper -cc1 option, stop using<br>
`-backend-option`, and stop passing anything to<br>
`cl::ParseCommandLineOptions()`.<br>
<span><br>
> 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:<br>
><br>
> 1. clang -target armv7-apple-ios -static -mlong-calls old.bc -o old<br>
> 2. clang -target armv7-apple-ios -static old.bc -o old<br>
><br>
> Here, old.bc is generated by an older version of clang that doesn't save "arm-long-calls" in the IR.<br>
<br>
</span>So in this example, #2 would behave the same, but #1 would stop<br>
enforcing -arm-long-calls (since old.bc doesn't have arm-long-calls<br>
encoded, and -mlong-calls only adds attributes to functions in IRGen).<br>
<br>
- AFAICT, there wasn't a supported way to pass -arm-long-calls to<br>
LTO until now (via attributes), so we won't regress behaviour<br>
there.<br>
- Outside of LTO, archiving bitcode and later codegen'ing from<br>
clang isn't supported, unless I'm missing something?<br>
<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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.</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
IMO we don't need to worry about backwards compatibility for #1, so<br>
I think: make it a binary option and stop using the `cl::opt`.<br>
<div><div><br>
><br>
> <a href="http://reviews.llvm.org/D9414" target="_blank">http://reviews.llvm.org/D9414</a><br>
><br>
> Files:<br>
> include/clang/Frontend/CodeGenOptions.h<br>
> lib/CodeGen/CGCall.cpp<br>
> lib/Frontend/CompilerInvocation.cpp<br>
> test/CodeGen/fn-attr.c<br>
><br>
> Index: include/clang/Frontend/CodeGenOptions.h<br>
> ===================================================================<br>
> --- include/clang/Frontend/CodeGenOptions.h<br>
> +++ include/clang/Frontend/CodeGenOptions.h<br>
> @@ -151,6 +151,9 @@<br>
> /// A list of command-line options to forward to the LLVM backend.<br>
> std::vector<std::string> BackendOptions;<br>
><br>
> + /// A list of function attributes to save to the IR.<br>
> + std::vector<std::pair<std::string, std::string>> FunctionAttributes;<br>
> +<br>
> /// A list of dependent libraries.<br>
> std::vector<std::string> DependentLibraries;<br>
><br>
> Index: lib/CodeGen/CGCall.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/CGCall.cpp<br>
> +++ lib/CodeGen/CGCall.cpp<br>
> @@ -1455,6 +1455,9 @@<br>
> FuncAttrs.addAttribute(llvm::Attribute::NoBuiltin);<br>
> } else {<br>
> // Attributes that should go on the function, but not the call site.<br>
> + for (auto &KV : CodeGenOpts.FunctionAttributes)<br>
> + FuncAttrs.addAttribute(KV.first, KV.second);<br>
> +<br>
> if (!CodeGenOpts.DisableFPElim) {<br>
> FuncAttrs.addAttribute("no-frame-pointer-elim", "false");<br>
> } else if (CodeGenOpts.OmitLeafFramePointer) {<br>
> Index: lib/Frontend/CompilerInvocation.cpp<br>
> ===================================================================<br>
> --- lib/Frontend/CompilerInvocation.cpp<br>
> +++ lib/Frontend/CompilerInvocation.cpp<br>
> @@ -340,6 +340,14 @@<br>
> }<br>
> }<br>
><br>
> +static void getFunctionAttributes(CodeGenOptions &Opts) {<br>
> + StringRef Opt = "-arm-long-calls", Key = Opt.drop_front(1);<br>
> +<br>
> + if (std::find(Opts.BackendOptions.begin(), Opts.BackendOptions.end(),<br>
> + Opt) != Opts.BackendOptions.end())<br>
> + Opts.FunctionAttributes.push_back(std::make_pair(Key, ""));<br>
> +}<br>
> +<br>
> static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,<br>
> DiagnosticsEngine &Diags,<br>
> const TargetOptions &TargetOpts) {<br>
> @@ -643,6 +651,8 @@<br>
> Args.getAllArgValues(OPT_fsanitize_recover_EQ), Diags,<br>
> Opts.SanitizeRecover);<br>
><br>
> + getFunctionAttributes(Opts);<br>
> +<br>
> return Success;<br>
> }<br>
><br>
> Index: test/CodeGen/fn-attr.c<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/CodeGen/fn-attr.c<br>
> @@ -0,0 +1,7 @@<br>
> +// RUN: %clang_cc1 -triple thumbv7-apple-ios5 -backend-option -arm-long-calls -emit-llvm -o - %s | FileCheck -check-prefix=LONGCALL %s<br>
> +// RUN: %clang_cc1 -triple thumbv7-apple-ios5 -emit-llvm -o - %s | FileCheck -check-prefix=NOLONGCALL %s<br>
> +<br>
> +// LONGCALL: attributes #0 = { {{.*}} "arm-long-calls<br>
> +// NOLONGCALL-NOT: attributes #0 = { {{.*}} "arm-long-calls<br>
> +<br>
> +int foo1(int a) { return a; }<br>
><br>
> EMAIL PREFERENCES<br>
> <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
</div></div>> <D9414.24762.patch><br>
<br>
</blockquote></div></div></div></blockquote></div>