r234668 - [Driver] Properly support -mglobal-merge using explicit options.
Ahmed Bougacha
ahmed.bougacha at gmail.com
Tue Apr 14 09:36:22 PDT 2015
On Tue, Apr 14, 2015 at 8:04 AM, Eric Christopher <echristo at gmail.com> wrote:
>
>
> On Tue, Apr 14, 2015 at 3:42 PM Ahmed Bougacha <ahmed.bougacha at gmail.com>
> wrote:
>>
>> On Tue, Apr 14, 2015 at 5:40 AM, Eric Christopher <echristo at gmail.com>
>> wrote:
>> > So, this is fine for now as a workaround, but using backend options is
>> > pretty terrible.
>>
>> Oh yes, agreed.
>>
>> > What we really need is a way of describing to the backend
>> > the particular options we'd like to communicate on and off
>> > programmatically
>> > without relying on cl::ParseCommandLineOptions. Currently I'm thinking
>> > of a
>> > target dependent struct that's passed along to TargetMachine creation.
>> > Other
>> > thoughts? I think Akira had mentioned a possibility of a string/string
>> > map
>> > as well.
>>
>> I might be missing the point, but where would this logic go? More
>> specifically, the driver (where we currently do this) is always one
>> step removed from TargetMachine creation, no? How do we pass this
>> through to -cc1, or to the LTO link invocation?
>>
>
> 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?
I'm heavily influenced by GlobalMerge, which is special (it's not
really a codegen option), but I have this gut feeling that framing
this as an LTO problem could help figuring out something clean. Plus,
getting it right for LTO means you get it right for non-LTO "for
free", and you have to deal with both sooner or later anyway.
> 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.
Makes sense.
> What you're doing is basically the
> string idea via "environment" parsing which isn't any better really.
Again, I totally agree with you, this commit is nothing more than a
temporary workaround (see FIXMEs in the LLVM side, I should have added
some in the clang side as well) while we figure out the solution. I
don't claim to solve anything other than fixing -mglobal-merge in
non-LTO!
>>
>> More generally, how does this relate to LTO? The usual annoying use
>> case is -msomething on one .cpp, -mno-something on the other, and LTO
>> on all this. From what I saw, this makes sense on some (?) but not all
>> such options.
>>
>
> 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?
That's the thing: I wonder if some of the options aren't incompatible.
You're right; for most of them, there's not much you can do if they
conflict, so a single codegen-time set of options is the right thing
to do.
By the way, the error at least makes it obvious to users that this
doesn't make sense, but it's better to catch this when compiling each
file anyway, if we don't already.
For instance, right above mglobal-merge in Tools.cpp, there's
mstrict-align and -arm-strict-align. I think we can do something
sensible with different options here, and you can reframe this as a
"different subtarget" problem, if that means anything.
Similarly, different -m[no-]global-merge flags make at least some
sense, if you interpret them as "control merging of globals internal
to this unit".
I think what I'm saying is basically orthogonal to the option-passing
problem: some of the things we use -backend-option for, we could
rewrite as attributes in the IR. And I'm not sure there's any real
reason that we haven't done that already (case in point: this
commit!).
>>
>> To get this working for GlobalMerge in particular, the best proposed
>> solution so far was having clang annotate each global with some
>> target-specific string attribute, much as we do for functions.
>>
>
> 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).
That sounds great! I'm ignorantly curious: how far is this from the
current state? My understanding is, we just forward options, but we
don't have logic like we do when driving -cc1, and you're saying we
should, right?
-Ahmed
>>
>> Currently, we don't support these mismatching flags, and from that
>> standpoint, either of the solutions, if I understand them correctly,
>> sound reasonable to me (with a slight preference for the struct,
>> because then at least, out-of-sync means broken build).
>>
>
> Right. That's my preference too.
>
>>
>> So, my real question is: when, if ever, does it make sense to support
>> that kind of thing?
>>
>
> I think I explained that above, or I've missed something. Let's keep this
> thread going and nail down the solution :)
>
> Thanks!
>
> -eric
>
>>
>> -Ahmed
>>
>> > -eric
>> >
>> > On Sat, Apr 11, 2015 at 1:16 AM Ahmed Bougacha
>> > <ahmed.bougacha at gmail.com>
>> > wrote:
>> >>
>> >> Author: ab
>> >> Date: Fri Apr 10 19:10:44 2015
>> >> New Revision: 234668
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=234668&view=rev
>> >> Log:
>> >> [Driver] Properly support -mglobal-merge using explicit options.
>> >>
>> >> Follow-up to r234666. With this, the -m[no-]global-merge options
>> >> have the expected behavior. Previously, -mglobal-merge was ignored,
>> >> and there was no way of enabling the optimization.
>> >>
>> >> Added:
>> >> cfe/trunk/test/Driver/mglobal-merge.c
>> >> - copied, changed from r234656,
>> >> cfe/trunk/test/Driver/mno-global-merge.c
>> >> Removed:
>> >> cfe/trunk/test/Driver/mno-global-merge.c
>> >> Modified:
>> >> cfe/trunk/include/clang/Driver/Options.td
>> >> cfe/trunk/include/clang/Frontend/CodeGenOptions.def
>> >> cfe/trunk/lib/CodeGen/BackendUtil.cpp
>> >> cfe/trunk/lib/Driver/Tools.cpp
>> >> cfe/trunk/lib/Frontend/CompilerInvocation.cpp
>> >>
>> >> Modified: cfe/trunk/include/clang/Driver/Options.td
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=234668&r1=234667&r2=234668&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- cfe/trunk/include/clang/Driver/Options.td (original)
>> >> +++ cfe/trunk/include/clang/Driver/Options.td Fri Apr 10 19:10:44 2015
>> >> @@ -1146,7 +1146,8 @@ def mfloat_abi_EQ : Joined<["-"], "mfloa
>> >> def mfpmath_EQ : Joined<["-"], "mfpmath=">, Group<m_Group>;
>> >> def mfpu_EQ : Joined<["-"], "mfpu=">, Group<m_Group>;
>> >> def mhwdiv_EQ : Joined<["-"], "mhwdiv=">, Group<m_Group>;
>> >> -def mglobal_merge : Flag<["-"], "mglobal-merge">, Group<m_Group>;
>> >> +def mglobal_merge : Flag<["-"], "mglobal-merge">, Group<m_Group>,
>> >> Flags<[CC1Option]>,
>> >> + HelpText<"Enable merging of globals">;
>> >> def mhard_float : Flag<["-"], "mhard-float">, Group<m_Group>;
>> >> def miphoneos_version_min_EQ : Joined<["-"],
>> >> "miphoneos-version-min=">,
>> >> Group<m_Group>;
>> >> def mios_version_min_EQ : Joined<["-"], "mios-version-min=">,
>> >>
>> >> Modified: cfe/trunk/include/clang/Frontend/CodeGenOptions.def
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CodeGenOptions.def?rev=234668&r1=234667&r2=234668&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- cfe/trunk/include/clang/Frontend/CodeGenOptions.def (original)
>> >> +++ cfe/trunk/include/clang/Frontend/CodeGenOptions.def Fri Apr 10
>> >> 19:10:44 2015
>> >> @@ -77,7 +77,6 @@ CODEGENOPT(NoExecStack , 1, 0) ///
>> >> CODEGENOPT(FatalWarnings , 1, 0) ///< Set when
>> >> -Wa,--fatal-warnings
>> >> is
>> >> ///< enabled.
>> >> CODEGENOPT(EnableSegmentedStacks , 1, 0) ///< Set when -fsplit-stack
>> >> is
>> >> enabled.
>> >> -CODEGENOPT(NoGlobalMerge , 1, 0) ///< Set when -mno-global-merge
>> >> is
>> >> enabled.
>> >> CODEGENOPT(NoImplicitFloat , 1, 0) ///< Set when -mno-implicit-float
>> >> is
>> >> enabled.
>> >> CODEGENOPT(NoInfsFPMath , 1, 0) ///< Assume FP arguments, results
>> >> not +-Inf.
>> >> CODEGENOPT(NoSignedZeros , 1, 0) ///< Allow ignoring the
>> >> signedness
>> >> of FP zero
>> >>
>> >> Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=234668&r1=234667&r2=234668&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)
>> >> +++ cfe/trunk/lib/CodeGen/BackendUtil.cpp Fri Apr 10 19:10:44 2015
>> >> @@ -436,8 +436,6 @@ TargetMachine *EmitAssemblyHelper::Creat
>> >> BackendArgs.push_back("-time-passes");
>> >> for (unsigned i = 0, e = CodeGenOpts.BackendOptions.size(); i != e;
>> >> ++i)
>> >> BackendArgs.push_back(CodeGenOpts.BackendOptions[i].c_str());
>> >> - if (CodeGenOpts.NoGlobalMerge)
>> >> - BackendArgs.push_back("-enable-global-merge=false");
>> >> BackendArgs.push_back(nullptr);
>> >> llvm::cl::ParseCommandLineOptions(BackendArgs.size() - 1,
>> >> BackendArgs.data());
>> >>
>> >> Modified: cfe/trunk/lib/Driver/Tools.cpp
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=234668&r1=234667&r2=234668&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- cfe/trunk/lib/Driver/Tools.cpp (original)
>> >> +++ cfe/trunk/lib/Driver/Tools.cpp Fri Apr 10 19:10:44 2015
>> >> @@ -861,12 +861,14 @@ void Clang::AddARMTargetArgs(const ArgLi
>> >> }
>> >> }
>> >>
>> >> - // Setting -mno-global-merge disables the codegen global merge pass.
>> >> Setting
>> >> - // -mglobal-merge has no effect as the pass is enabled by default.
>> >> + // Forward the -mglobal-merge option for explicit control over the
>> >> pass.
>> >> if (Arg *A = Args.getLastArg(options::OPT_mglobal_merge,
>> >> options::OPT_mno_global_merge)) {
>> >> + CmdArgs.push_back("-backend-option");
>> >> if (A->getOption().matches(options::OPT_mno_global_merge))
>> >> - CmdArgs.push_back("-mno-global-merge");
>> >> + CmdArgs.push_back("-arm-global-merge=false");
>> >> + else
>> >> + CmdArgs.push_back("-arm-global-merge=true");
>> >> }
>> >>
>> >> if (!Args.hasFlag(options::OPT_mimplicit_float,
>> >> @@ -957,12 +959,14 @@ void Clang::AddAArch64TargetArgs(const A
>> >> CmdArgs.push_back("-aarch64-fix-cortex-a53-835769=1");
>> >> }
>> >>
>> >> - // Setting -mno-global-merge disables the codegen global merge pass.
>> >> Setting
>> >> - // -mglobal-merge has no effect as the pass is enabled by default.
>> >> + // Forward the -mglobal-merge option for explicit control over the
>> >> pass.
>> >> if (Arg *A = Args.getLastArg(options::OPT_mglobal_merge,
>> >> options::OPT_mno_global_merge)) {
>> >> + CmdArgs.push_back("-backend-option");
>> >> if (A->getOption().matches(options::OPT_mno_global_merge))
>> >> - CmdArgs.push_back("-mno-global-merge");
>> >> + CmdArgs.push_back("-aarch64-global-merge=false");
>> >> + else
>> >> + CmdArgs.push_back("-aarch64-global-merge=true");
>> >> }
>> >>
>> >> if (Args.hasArg(options::OPT_ffixed_x18)) {
>> >>
>> >> Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=234668&r1=234667&r2=234668&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
>> >> +++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Fri Apr 10 19:10:44
>> >> 2015
>> >> @@ -455,7 +455,6 @@ static bool ParseCodeGenArgs(CodeGenOpti
>> >> Opts.NoZeroInitializedInBSS =
>> >> Args.hasArg(OPT_mno_zero_initialized_in_bss);
>> >> Opts.BackendOptions = Args.getAllArgValues(OPT_backend_option);
>> >> Opts.NumRegisterParameters = getLastArgIntValue(Args, OPT_mregparm,
>> >> 0,
>> >> Diags);
>> >> - Opts.NoGlobalMerge = Args.hasArg(OPT_mno_global_merge);
>> >> Opts.NoExecStack = Args.hasArg(OPT_mno_exec_stack);
>> >> Opts.FatalWarnings = Args.hasArg(OPT_massembler_fatal_warnings);
>> >> Opts.EnableSegmentedStacks = Args.hasArg(OPT_split_stacks);
>> >>
>> >> Copied: cfe/trunk/test/Driver/mglobal-merge.c (from r234656,
>> >> cfe/trunk/test/Driver/mno-global-merge.c)
>> >> URL:
>> >>
>> >> 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
>> >>
>> >>
>> >> ==============================================================================
>> >> --- cfe/trunk/test/Driver/mno-global-merge.c (original)
>> >> +++ cfe/trunk/test/Driver/mglobal-merge.c Fri Apr 10 19:10:44 2015
>> >> @@ -1,20 +1,40 @@
>> >> -// RUN: %clang -target armv7-apple-darwin10 \
>> >> -// RUN: -mno-global-merge -### -fsyntax-only %s 2> %t
>> >> -// RUN: FileCheck --check-prefix=CHECK-NGM < %t %s
>> >> +// RUN: %clang -target armv7-unknown-unknown -### -fsyntax-only %s 2>
>> >> %t
>> >> \
>> >> +// RUN: -mno-global-merge
>> >> +// RUN: FileCheck --check-prefix=CHECK-NGM-ARM < %t %s
>> >>
>> >> -// RUN: %clang -target arm64-apple-ios7 \
>> >> -// RUN: -mno-global-merge -### -fsyntax-only %s 2> %t
>> >> -// RUN: FileCheck --check-prefix=CHECK-NGM < %t %s
>> >> +// RUN: %clang -target aarch64-unknown-unknown -### -fsyntax-only %s
>> >> 2>
>> >> %t \
>> >> +// RUN: -mno-global-merge
>> >> +// RUN: FileCheck --check-prefix=CHECK-NGM-AARCH64 < %t %s
>> >>
>> >> -// CHECK-NGM: "-mno-global-merge"
>> >> +// RUN: %clang -target x86_64-unknown-unknown -### -fsyntax-only %s 2>
>> >> %t
>> >> \
>> >> +// RUN: -mno-global-merge
>> >> +// RUN: FileCheck --check-prefix=CHECK-NONE < %t %s
>> >>
>> >> -// RUN: %clang -target armv7-apple-darwin10 \
>> >> -// RUN: -mglobal-merge -### -fsyntax-only %s 2> %t
>> >> -// RUN: FileCheck --check-prefix=CHECK-GM < %t %s
>> >> +// CHECK-NGM-ARM: "-backend-option" "-arm-global-merge=false"
>> >> +// CHECK-NGM-AARCH64: "-backend-option" "-aarch64-global-merge=false"
>> >>
>> >> -// RUN: %clang -target arm64-apple-ios7 \
>> >> -// RUN: -mglobal-merge -### -fsyntax-only %s 2> %t
>> >> -// RUN: FileCheck --check-prefix=CHECK-GM < %t %s
>> >> +// RUN: %clang -target armv7-unknown-unknown -### -fsyntax-only %s 2>
>> >> %t
>> >> \
>> >> +// RUN: -mglobal-merge
>> >> +// RUN: FileCheck --check-prefix=CHECK-GM-ARM < %t %s
>> >>
>> >> -// CHECK-GM-NOT: "-mglobal-merge"
>> >> +// RUN: %clang -target aarch64-unknown-unknown -### -fsyntax-only %s
>> >> 2>
>> >> %t \
>> >> +// RUN: -mglobal-merge
>> >> +// RUN: FileCheck --check-prefix=CHECK-GM-AARCH64 < %t %s
>> >>
>> >> +// RUN: %clang -target x86_64-unknown-unknown -### -fsyntax-only %s 2>
>> >> %t
>> >> \
>> >> +// RUN: -mglobal-merge
>> >> +// RUN: FileCheck --check-prefix=CHECK-NONE < %t %s
>> >> +
>> >> +// CHECK-GM-ARM: "-backend-option" "-arm-global-merge=true"
>> >> +// CHECK-GM-AARCH64: "-backend-option" "-aarch64-global-merge=true"
>> >> +
>> >> +// RUN: %clang -target armv7-unknown-unknown -### -fsyntax-only %s 2>
>> >> %t
>> >> +// RUN: FileCheck --check-prefix=CHECK-NONE < %t %s
>> >> +
>> >> +// RUN: %clang -target aarch64-unknown-unknown -### -fsyntax-only %s
>> >> 2>
>> >> %t
>> >> +// RUN: FileCheck --check-prefix=CHECK-NONE < %t %s
>> >> +
>> >> +// RUN: %clang -target x86_64-unknown-unknown -### -fsyntax-only %s 2>
>> >> %t
>> >> +// RUN: FileCheck --check-prefix=CHECK-NONE < %t %s
>> >> +
>> >> +// CHECK-NONE-NOT: -global-merge=
>> >>
>> >> Removed: cfe/trunk/test/Driver/mno-global-merge.c
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/mno-global-merge.c?rev=234667&view=auto
>> >>
>> >>
>> >> ==============================================================================
>> >> --- cfe/trunk/test/Driver/mno-global-merge.c (original)
>> >> +++ cfe/trunk/test/Driver/mno-global-merge.c (removed)
>> >> @@ -1,20 +0,0 @@
>> >> -// RUN: %clang -target armv7-apple-darwin10 \
>> >> -// RUN: -mno-global-merge -### -fsyntax-only %s 2> %t
>> >> -// RUN: FileCheck --check-prefix=CHECK-NGM < %t %s
>> >> -
>> >> -// RUN: %clang -target arm64-apple-ios7 \
>> >> -// RUN: -mno-global-merge -### -fsyntax-only %s 2> %t
>> >> -// RUN: FileCheck --check-prefix=CHECK-NGM < %t %s
>> >> -
>> >> -// CHECK-NGM: "-mno-global-merge"
>> >> -
>> >> -// RUN: %clang -target armv7-apple-darwin10 \
>> >> -// RUN: -mglobal-merge -### -fsyntax-only %s 2> %t
>> >> -// RUN: FileCheck --check-prefix=CHECK-GM < %t %s
>> >> -
>> >> -// RUN: %clang -target arm64-apple-ios7 \
>> >> -// RUN: -mglobal-merge -### -fsyntax-only %s 2> %t
>> >> -// RUN: FileCheck --check-prefix=CHECK-GM < %t %s
>> >> -
>> >> -// CHECK-GM-NOT: "-mglobal-merge"
>> >> -
>> >>
>> >>
>> >> _______________________________________________
>> >> cfe-commits mailing list
>> >> cfe-commits at cs.uiuc.edu
>> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list