Page MenuHomePhabricator

Pass -mglobal-merge as a module flag metadata.
Needs ReviewPublic

Authored by ab on Feb 27 2015, 6:52 PM.

Details

Summary

Per title, we used to pass -mno-global-merge when specified, and enable GlobalMerge always (leaving it to LLVM to decide whether to actually enable it: it does, on ARM and AArch64, for -O1 and above).

This addresses two problems (I'm fine with splitting the patch if desired):

  • with LTO, passing -mno-global-merge didn't do anything, since LTO implies -O3 from the backend's standpoint.
  • GlobalMerge was enabled by -O1. Instead, on AArch64, enable it at -O3, or when -mglobal-merge is specified. (the goal is to have it predicated by -O3 on ARM as well, but I still need to measure before doing that.)

Now, -mglobal-merge and -mno-global-merge are both explicitly passed by the driver.
They are then passed to the backend using module flag metadata (bikeshedding time: name is prose, and value is string. We can change both, and AFAICT there's no similar backend flag to be consistent with, yet.)

Thanks!
-Ahmed

Diff Detail

Event Timeline

ab updated this revision to Diff 20925.Feb 27 2015, 6:52 PM
ab retitled this revision from to Pass -mglobal-merge as a module flag metadata..
ab updated this object.
ab edited the test plan for this revision. (Show Details)
ab added a reviewer: dexonsmith.
ab added a subscriber: Unknown Object (MLST).
ab added a reviewer: ahatanak.Feb 27 2015, 7:02 PM
ahatanak edited edge metadata.Mar 1 2015, 1:57 PM

Just one question:

Should it be an error if two files compiled with -mglobal-merge and -mno-global-merge are linked? With this patch applied, the merged IR (foo3.ll) gets "Enable Global Merge"="true" in such case.

$ clang foo1.c -o foo1.ll -mglobal-merge ...
$ clang foo2.c -o foo2.ll -mno-global-merge ...
$ lvm-link foo1.ll foo2.ll -o foo3.ll

lib/CodeGen/CodeGenModule.cpp
3617

Do you need to pass LLVMContext here? I saw a build error when I applied this patch.

ab planned changes to this revision.Mar 1 2015, 2:09 PM

Good point, both issues are caused by a broken last-minute change: I made the module flag to be emitted only if "true", because I didn't want "false" flags littered in every module emitted (even on targets where it doesn't make sense).

I'll just revert to always emitting the flag, whether true or false, but only on ARM/AArch64. Does that sound OK?

-Ahmed

ab added a comment.Mar 1 2015, 2:11 PM

And yes, in case that wasn't clear, conflicting flags should be an error.

How about the following case:

$ clang foo1.c -S -arch arm64 -o foo1.ll -O3
$ clang foo2.c -S -arch arm64 -o foo2.ll -mno-global-merge

foo1.ll would get "Enable Global Merge"="true" while foo2.ll would get "Enable Global Merge"="false" and linking the two modules would be an error. Should the linker emit a linked module with "Enable Global Merge"="false" without any errors in this case?

My question is, if -mno-global-merge/-mglobal-merge is provided on the command line, does it mean global-merge pass should be disabled or enabled regardless of what the optimization level is?

ab updated this revision to Diff 21014.Mar 2 2015, 9:55 AM
ab edited edge metadata.
  • Only emit flag when GlobalMerge is disabled (per Akira and Duncan's reviews)

I added a target check to only emit the flags on ARM/AArch64. I'm fine with emitting the flag no matter the target, if desired (less brittle), but I personally don't like adding noise to every module ever, hence hiding it.

dexonsmith edited edge metadata.Mar 2 2015, 12:40 PM

+echristo

ab updated this revision to Diff 21060.Mar 2 2015, 4:12 PM
ab edited edge metadata.
  • Switch to a "Global Merge" flag, enabled by default.
  • Generate i32 values instead of strings.
  • Add X86 tests (to make sure we don't emit the flags there).
ab added a reviewer: echristo.Mar 5 2015, 1:36 PM

+echristo as a reviewer, to chime in on the use of flags.

-Ahmed

echristo edited edge metadata.Mar 13 2015, 3:23 PM

Hrm. What do you think about requiring that flags like this for LTO are just required to be passed on the command line rather than left in the module and subject to module merge? If so, you can just pass the option to the backend via TargetOptions which will put it on the TargetMachine and make it easy to grab for the pass manager.

Thoughts?

-eric

No, you probably haven't. I was seeing it as clang doing to lto link of the module together and then codegen based on that (which means it would have the options), but...

That said, I think the general problem is more specific. I.e. how do you specify -msse3 as part of the default code generation flags when you do lto?

The C++ interface has addAttr (which is painful in that it requires, as you say, every linker to understand llvm's command line interface), but this is also pretty painful:

const void *compile(size_t *length,
                    bool disableOpt,
                    bool disableInline,
                    bool disableGVNLoadPRE,
                    bool disableVectorization,
                    std::string &errMsg);

because, you know, all optimizations, inlining, gvnloadpre, and vectorization are all anyone care about :)

Realize this has dovetailed into "let's solve the general problem" but I am curious. The gold plugin's methods aren't much better.

Or am I missing something?

-eric