<div dir="ltr"><br><br><div class="gmail_quote">On Tue, Apr 14, 2015 at 3:42 PM Ahmed Bougacha <<a href="mailto:ahmed.bougacha@gmail.com">ahmed.bougacha@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Apr 14, 2015 at 5:40 AM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:<br>
> So, this is fine for now as a workaround, but using backend options is<br>
> pretty terrible.<br>
<br>
Oh yes, agreed.<br>
<br>
> What we really need is a way of describing to the backend<br>
> the particular options we'd like to communicate on and off programmatically<br>
> without relying on cl::ParseCommandLineOptions. Currently I'm thinking of a<br>
> target dependent struct that's passed along to TargetMachine creation. Other<br>
> thoughts? I think Akira had mentioned a possibility of a string/string map<br>
> as well.<br>
<br>
I might be missing the point, but where would this logic go? More<br>
specifically, the driver (where we currently do this) is always one<br>
step removed from TargetMachine creation, no? How do we pass this<br>
through to -cc1, or to the LTO link invocation?<br>
<br></blockquote><div><br></div><div>Going to separate this out to the -cc1 side of things. LTO is terrible for passing code generation options around. I've got some ideas there, but you haven't really solved the LTO problem anyhow have you?</div><div><br></div><div>For -cc1 I'd just do what we do with a lot of things, pass a flag to -cc1 and don't worry about it. Parse it via the cc1 options and then use that to fill in whatever data structure we want. What you're doing is basically the string idea via "environment" parsing which isn't any better really.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
More generally, how does this relate to LTO? The usual annoying use<br>
case is -msomething on one .cpp, -mno-something on the other, and LTO<br>
on all this. From what I saw, this makes sense on some (?) but not all<br>
such options.<br>
<br></blockquote><div><br></div><div>It makes sense on no fronts really. This is why I want global code generation options for LTO. Either you agree on everything which is great, or you don't at which point you, what? Error? Pick one?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
To get this working for GlobalMerge in particular, the best proposed<br>
solution so far was having clang annotate each global with some<br>
target-specific string attribute, much as we do for functions.<br>
<br></blockquote><div><br></div><div>Yeah, I really don't like this. I'd rather the lto code be able to take options to change the pass manager. We already support plugin-opt for the gold plugin, there's no real reason we couldn't expand that further (or, we start driving LTO via clang which is my pie in the sky idea on how this should work, but that's a lot more work that I'm not signing up to do myself right now).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Currently, we don't support these mismatching flags, and from that<br>
standpoint, either of the solutions, if I understand them correctly,<br>
sound reasonable to me (with a slight preference for the struct,<br>
because then at least, out-of-sync means broken build).<br>
<br></blockquote><div><br></div><div>Right. That's my preference too.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
So, my real question is: when, if ever, does it make sense to support<br>
that kind of thing?<br>
<br></blockquote><div><br></div><div>I think I explained that above, or I've missed something. Let's keep this thread going and nail down the solution :)</div><div><br></div><div>Thanks!</div><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
-Ahmed<br>
<br>
> -eric<br>
><br>
> On Sat, Apr 11, 2015 at 1:16 AM Ahmed Bougacha <<a href="mailto:ahmed.bougacha@gmail.com" target="_blank">ahmed.bougacha@gmail.com</a>><br>
> wrote:<br>
>><br>
>> Author: ab<br>
>> Date: Fri Apr 10 19:10:44 2015<br>
>> New Revision: 234668<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=234668&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=234668&view=rev</a><br>
>> Log:<br>
>> [Driver] Properly support -mglobal-merge using explicit options.<br>
>><br>
>> Follow-up to r234666. With this, the -m[no-]global-merge options<br>
>> have the expected behavior. Previously, -mglobal-merge was ignored,<br>
>> and there was no way of enabling the optimization.<br>
>><br>
>> Added:<br>
>> cfe/trunk/test/Driver/mglobal-merge.c<br>
>> - copied, changed from r234656,<br>
>> cfe/trunk/test/Driver/mno-global-merge.c<br>
>> Removed:<br>
>> cfe/trunk/test/Driver/mno-global-merge.c<br>
>> Modified:<br>
>> cfe/trunk/include/clang/Driver/Options.td<br>
>> cfe/trunk/include/clang/Frontend/CodeGenOptions.def<br>
>> cfe/trunk/lib/CodeGen/BackendUtil.cpp<br>
>> cfe/trunk/lib/Driver/Tools.cpp<br>
>> cfe/trunk/lib/Frontend/CompilerInvocation.cpp<br>
>><br>
>> Modified: cfe/trunk/include/clang/Driver/Options.td<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=234668&r1=234667&r2=234668&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=234668&r1=234667&r2=234668&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/include/clang/Driver/Options.td (original)<br>
>> +++ cfe/trunk/include/clang/Driver/Options.td Fri Apr 10 19:10:44 2015<br>
>> @@ -1146,7 +1146,8 @@ def mfloat_abi_EQ : Joined<["-"], "mfloa<br>
>> def mfpmath_EQ : Joined<["-"], "mfpmath=">, Group<m_Group>;<br>
>> def mfpu_EQ : Joined<["-"], "mfpu=">, Group<m_Group>;<br>
>> def mhwdiv_EQ : Joined<["-"], "mhwdiv=">, Group<m_Group>;<br>
>> -def mglobal_merge : Flag<["-"], "mglobal-merge">, Group<m_Group>;<br>
>> +def mglobal_merge : Flag<["-"], "mglobal-merge">, Group<m_Group>,<br>
>> Flags<[CC1Option]>,<br>
>> + HelpText<"Enable merging of globals">;<br>
>> def mhard_float : Flag<["-"], "mhard-float">, Group<m_Group>;<br>
>> def miphoneos_version_min_EQ : Joined<["-"], "miphoneos-version-min=">,<br>
>> Group<m_Group>;<br>
>> def mios_version_min_EQ : Joined<["-"], "mios-version-min=">,<br>
>><br>
>> Modified: cfe/trunk/include/clang/Frontend/CodeGenOptions.def<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CodeGenOptions.def?rev=234668&r1=234667&r2=234668&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CodeGenOptions.def?rev=234668&r1=234667&r2=234668&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/include/clang/Frontend/CodeGenOptions.def (original)<br>
>> +++ cfe/trunk/include/clang/Frontend/CodeGenOptions.def Fri Apr 10<br>
>> 19:10:44 2015<br>
>> @@ -77,7 +77,6 @@ CODEGENOPT(NoExecStack , 1, 0) ///<br>
>> CODEGENOPT(FatalWarnings , 1, 0) ///< Set when -Wa,--fatal-warnings<br>
>> is<br>
>> ///< enabled.<br>
>> CODEGENOPT(EnableSegmentedStacks , 1, 0) ///< Set when -fsplit-stack is<br>
>> enabled.<br>
>> -CODEGENOPT(NoGlobalMerge , 1, 0) ///< Set when -mno-global-merge is<br>
>> enabled.<br>
>> CODEGENOPT(NoImplicitFloat , 1, 0) ///< Set when -mno-implicit-float is<br>
>> enabled.<br>
>> CODEGENOPT(NoInfsFPMath , 1, 0) ///< Assume FP arguments, results<br>
>> not +-Inf.<br>
>> CODEGENOPT(NoSignedZeros , 1, 0) ///< Allow ignoring the signedness<br>
>> of FP zero<br>
>><br>
>> Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=234668&r1=234667&r2=234668&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=234668&r1=234667&r2=234668&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)<br>
>> +++ cfe/trunk/lib/CodeGen/BackendUtil.cpp Fri Apr 10 19:10:44 2015<br>
>> @@ -436,8 +436,6 @@ TargetMachine *EmitAssemblyHelper::Creat<br>
>> BackendArgs.push_back("-time-passes");<br>
>> for (unsigned i = 0, e = CodeGenOpts.BackendOptions.size(); i != e;<br>
>> ++i)<br>
>> BackendArgs.push_back(CodeGenOpts.BackendOptions[i].c_str());<br>
>> - if (CodeGenOpts.NoGlobalMerge)<br>
>> - BackendArgs.push_back("-enable-global-merge=false");<br>
>> BackendArgs.push_back(nullptr);<br>
>> llvm::cl::ParseCommandLineOptions(BackendArgs.size() - 1,<br>
>> BackendArgs.data());<br>
>><br>
>> Modified: cfe/trunk/lib/Driver/Tools.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=234668&r1=234667&r2=234668&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=234668&r1=234667&r2=234668&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/lib/Driver/Tools.cpp (original)<br>
>> +++ cfe/trunk/lib/Driver/Tools.cpp Fri Apr 10 19:10:44 2015<br>
>> @@ -861,12 +861,14 @@ void Clang::AddARMTargetArgs(const ArgLi<br>
>> }<br>
>> }<br>
>><br>
>> - // Setting -mno-global-merge disables the codegen global merge pass.<br>
>> Setting<br>
>> - // -mglobal-merge has no effect as the pass is enabled by default.<br>
>> + // Forward the -mglobal-merge option for explicit control over the<br>
>> pass.<br>
>> if (Arg *A = Args.getLastArg(options::OPT_mglobal_merge,<br>
>> options::OPT_mno_global_merge)) {<br>
>> + CmdArgs.push_back("-backend-option");<br>
>> if (A->getOption().matches(options::OPT_mno_global_merge))<br>
>> - CmdArgs.push_back("-mno-global-merge");<br>
>> + CmdArgs.push_back("-arm-global-merge=false");<br>
>> + else<br>
>> + CmdArgs.push_back("-arm-global-merge=true");<br>
>> }<br>
>><br>
>> if (!Args.hasFlag(options::OPT_mimplicit_float,<br>
>> @@ -957,12 +959,14 @@ void Clang::AddAArch64TargetArgs(const A<br>
>> CmdArgs.push_back("-aarch64-fix-cortex-a53-835769=1");<br>
>> }<br>
>><br>
>> - // Setting -mno-global-merge disables the codegen global merge pass.<br>
>> Setting<br>
>> - // -mglobal-merge has no effect as the pass is enabled by default.<br>
>> + // Forward the -mglobal-merge option for explicit control over the<br>
>> pass.<br>
>> if (Arg *A = Args.getLastArg(options::OPT_mglobal_merge,<br>
>> options::OPT_mno_global_merge)) {<br>
>> + CmdArgs.push_back("-backend-option");<br>
>> if (A->getOption().matches(options::OPT_mno_global_merge))<br>
>> - CmdArgs.push_back("-mno-global-merge");<br>
>> + CmdArgs.push_back("-aarch64-global-merge=false");<br>
>> + else<br>
>> + CmdArgs.push_back("-aarch64-global-merge=true");<br>
>> }<br>
>><br>
>> if (Args.hasArg(options::OPT_ffixed_x18)) {<br>
>><br>
>> Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=234668&r1=234667&r2=234668&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=234668&r1=234667&r2=234668&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)<br>
>> +++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Fri Apr 10 19:10:44 2015<br>
>> @@ -455,7 +455,6 @@ static bool ParseCodeGenArgs(CodeGenOpti<br>
>> Opts.NoZeroInitializedInBSS =<br>
>> Args.hasArg(OPT_mno_zero_initialized_in_bss);<br>
>> Opts.BackendOptions = Args.getAllArgValues(OPT_backend_option);<br>
>> Opts.NumRegisterParameters = getLastArgIntValue(Args, OPT_mregparm, 0,<br>
>> Diags);<br>
>> - Opts.NoGlobalMerge = Args.hasArg(OPT_mno_global_merge);<br>
>> Opts.NoExecStack = Args.hasArg(OPT_mno_exec_stack);<br>
>> Opts.FatalWarnings = Args.hasArg(OPT_massembler_fatal_warnings);<br>
>> Opts.EnableSegmentedStacks = Args.hasArg(OPT_split_stacks);<br>
>><br>
>> Copied: cfe/trunk/test/Driver/mglobal-merge.c (from r234656,<br>
>> cfe/trunk/test/Driver/mno-global-merge.c)<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/mglobal-merge.c?p2=cfe/trunk/test/Driver/mglobal-merge.c&p1=cfe/trunk/test/Driver/mno-global-merge.c&r1=234656&r2=234668&rev=234668&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/mglobal-merge.c?p2=cfe/trunk/test/Driver/mglobal-merge.c&p1=cfe/trunk/test/Driver/mno-global-merge.c&r1=234656&r2=234668&rev=234668&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/test/Driver/mno-global-merge.c (original)<br>
>> +++ cfe/trunk/test/Driver/mglobal-merge.c Fri Apr 10 19:10:44 2015<br>
>> @@ -1,20 +1,40 @@<br>
>> -// RUN: %clang -target armv7-apple-darwin10 \<br>
>> -// RUN: -mno-global-merge -### -fsyntax-only %s 2> %t<br>
>> -// RUN: FileCheck --check-prefix=CHECK-NGM < %t %s<br>
>> +// RUN: %clang -target armv7-unknown-unknown -### -fsyntax-only %s 2> %t<br>
>> \<br>
>> +// RUN: -mno-global-merge<br>
>> +// RUN: FileCheck --check-prefix=CHECK-NGM-ARM < %t %s<br>
>><br>
>> -// RUN: %clang -target arm64-apple-ios7 \<br>
>> -// RUN: -mno-global-merge -### -fsyntax-only %s 2> %t<br>
>> -// RUN: FileCheck --check-prefix=CHECK-NGM < %t %s<br>
>> +// RUN: %clang -target aarch64-unknown-unknown -### -fsyntax-only %s 2><br>
>> %t \<br>
>> +// RUN: -mno-global-merge<br>
>> +// RUN: FileCheck --check-prefix=CHECK-NGM-AARCH64 < %t %s<br>
>><br>
>> -// CHECK-NGM: "-mno-global-merge"<br>
>> +// RUN: %clang -target x86_64-unknown-unknown -### -fsyntax-only %s 2> %t<br>
>> \<br>
>> +// RUN: -mno-global-merge<br>
>> +// RUN: FileCheck --check-prefix=CHECK-NONE < %t %s<br>
>><br>
>> -// RUN: %clang -target armv7-apple-darwin10 \<br>
>> -// RUN: -mglobal-merge -### -fsyntax-only %s 2> %t<br>
>> -// RUN: FileCheck --check-prefix=CHECK-GM < %t %s<br>
>> +// CHECK-NGM-ARM: "-backend-option" "-arm-global-merge=false"<br>
>> +// CHECK-NGM-AARCH64: "-backend-option" "-aarch64-global-merge=false"<br>
>><br>
>> -// RUN: %clang -target arm64-apple-ios7 \<br>
>> -// RUN: -mglobal-merge -### -fsyntax-only %s 2> %t<br>
>> -// RUN: FileCheck --check-prefix=CHECK-GM < %t %s<br>
>> +// RUN: %clang -target armv7-unknown-unknown -### -fsyntax-only %s 2> %t<br>
>> \<br>
>> +// RUN: -mglobal-merge<br>
>> +// RUN: FileCheck --check-prefix=CHECK-GM-ARM < %t %s<br>
>><br>
>> -// CHECK-GM-NOT: "-mglobal-merge"<br>
>> +// RUN: %clang -target aarch64-unknown-unknown -### -fsyntax-only %s 2><br>
>> %t \<br>
>> +// RUN: -mglobal-merge<br>
>> +// RUN: FileCheck --check-prefix=CHECK-GM-AARCH64 < %t %s<br>
>><br>
>> +// RUN: %clang -target x86_64-unknown-unknown -### -fsyntax-only %s 2> %t<br>
>> \<br>
>> +// RUN: -mglobal-merge<br>
>> +// RUN: FileCheck --check-prefix=CHECK-NONE < %t %s<br>
>> +<br>
>> +// CHECK-GM-ARM: "-backend-option" "-arm-global-merge=true"<br>
>> +// CHECK-GM-AARCH64: "-backend-option" "-aarch64-global-merge=true"<br>
>> +<br>
>> +// RUN: %clang -target armv7-unknown-unknown -### -fsyntax-only %s 2> %t<br>
>> +// RUN: FileCheck --check-prefix=CHECK-NONE < %t %s<br>
>> +<br>
>> +// RUN: %clang -target aarch64-unknown-unknown -### -fsyntax-only %s 2><br>
>> %t<br>
>> +// RUN: FileCheck --check-prefix=CHECK-NONE < %t %s<br>
>> +<br>
>> +// RUN: %clang -target x86_64-unknown-unknown -### -fsyntax-only %s 2> %t<br>
>> +// RUN: FileCheck --check-prefix=CHECK-NONE < %t %s<br>
>> +<br>
>> +// CHECK-NONE-NOT: -global-merge=<br>
>><br>
>> Removed: cfe/trunk/test/Driver/mno-global-merge.c<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/mno-global-merge.c?rev=234667&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/mno-global-merge.c?rev=234667&view=auto</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/test/Driver/mno-global-merge.c (original)<br>
>> +++ cfe/trunk/test/Driver/mno-global-merge.c (removed)<br>
>> @@ -1,20 +0,0 @@<br>
>> -// RUN: %clang -target armv7-apple-darwin10 \<br>
>> -// RUN: -mno-global-merge -### -fsyntax-only %s 2> %t<br>
>> -// RUN: FileCheck --check-prefix=CHECK-NGM < %t %s<br>
>> -<br>
>> -// RUN: %clang -target arm64-apple-ios7 \<br>
>> -// RUN: -mno-global-merge -### -fsyntax-only %s 2> %t<br>
>> -// RUN: FileCheck --check-prefix=CHECK-NGM < %t %s<br>
>> -<br>
>> -// CHECK-NGM: "-mno-global-merge"<br>
>> -<br>
>> -// RUN: %clang -target armv7-apple-darwin10 \<br>
>> -// RUN: -mglobal-merge -### -fsyntax-only %s 2> %t<br>
>> -// RUN: FileCheck --check-prefix=CHECK-GM < %t %s<br>
>> -<br>
>> -// RUN: %clang -target arm64-apple-ios7 \<br>
>> -// RUN: -mglobal-merge -### -fsyntax-only %s 2> %t<br>
>> -// RUN: FileCheck --check-prefix=CHECK-GM < %t %s<br>
>> -<br>
>> -// CHECK-GM-NOT: "-mglobal-merge"<br>
>> -<br>
>><br>
>><br>
>> _______________________________________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>