[PATCH] Pass -mglobal-merge as a module flag metadata.

[Ahmed Bougacha]

On Mon, Mar 2, 2015 at 12:38 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
> +echristo
>
>> On 2015-Mar-02, at 09:55, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:
>>
>> - 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.
>
> Now the logic is what I expected, but I'm less confident that it's the
> best path.
>
> The logic shakes out like this:
>
>   - Change the driver to *always* specify -m{no-,}global-merge.
>   - In -cc1:
>       - If -mglobal-merge: don't set any flags.
>       - If -mno-global-merge: add `Error` flag to explicitly disable it,
>         if this is a relevant target.
>   - When linking modules:
>       - If any module has the flag, it's disabled in the linked module.
>   - When running codegen: run global-merge (if possible) unless it has
>     been disabled.
>
> Maybe I haven't had enough sleep, but this other logic seems more
> appealing to me now.
>
>   - Use the old driver logic (pass the option through only if
>     specified).

I believe this is already the case with the patch, no?  Pass the
option (-m or -mno) only if specified (if getLastArg found something).

>   - In -cc1:
>       - -mno-global-merge: add `Override` flag to disable it.
>       - -mglobal-merge: add `Error` flag to enable it.
>       - (Otherwise, set the flag based on optimization level?  Or don't
>         set a flag?)

Note that in real life (tm), the flag isn't passed that often (if
ever: it used to be broken for a while, until someone eventually
noticed).
The interesting case is deciding whether to enable it or not in LTO.
If you compile with -O1/-Os, you need a way to tell the LTO backend to
respect that; it always uses -O3  (a generic "original optimization
level" doesn't really work, because you might compile files with
different levels, and I'm not sure how you can merge that).
That's why I want to set a flag based on the clang optimization level.
Does that make sense?

>   - When linking modules:
>       - If any module disabled it, it's disabled in the linked module.
>       - Else, if any module has enabled it, it's enabled in the linked
>         module.
>       - Else, the linked module has neither flag.

To be clear, this has the same behavior as the current patch, if on
the LLVM side the pass is enabled by default when the flag is missing,
right?

The logic SGTM, the 'Override' seems cleaner indeed.

>   - When running codegen: respect the flag if present; otherwise, use
>     old logic (optimization level?).

Yep, and yep (-O1 backend level).

> @Eric/Akira/Ahmed, any thoughts?
>
> @Ahmed, if we stick with this logic, I have some nitpicks below on the
> spelling.
>
> Also, was there an LLVM patch that went with this?  Can you attach it to
> the same thread?

Yep, separate under http://reviews.llvm.org/D7969.  Let's discuss
here, I'll update that one when we come up with something.

>
>> Index: lib/CodeGen/CodeGenModule.cpp
>> ===================================================================
>> --- lib/CodeGen/CodeGenModule.cpp
>> +++ lib/CodeGen/CodeGenModule.cpp
>> @@ -418,6 +418,8 @@
>>    EmitVersionIdentMetadata();
>>
>>    EmitTargetMetadata();
>> +
>> +  EmitBackendOptionsMetadata();
>>  }
>>
>>  void CodeGenModule::UpdateCompletedType(const TagDecl *TD) {
>> @@ -3609,6 +3611,18 @@
>>    }
>>  }
>>
>> +void CodeGenModule::EmitBackendOptionsMetadata() {
>> +  // Only emit the "Global Merge" flag when it disables it, and only on targets
>> +  // that support it (ARM/AArch64).
>> +  llvm::Triple::ArchType Arch = Context.getTargetInfo().getTriple().getArch();
>> +  if ((Arch == llvm::Triple::arm || Arch == llvm::Triple::armeb ||
>> +       Arch == llvm::Triple::thumb || Arch == llvm::Triple::thumbeb ||
>> +       Arch == llvm::Triple::aarch64 || Arch == llvm::Triple::aarch64_be) &&
>> +      getCodeGenOpts().NoGlobalMerge)
>> +    getModule().addModuleFlag(llvm::Module::Error, "Enable Global Merge",
>> +                              llvm::MDString::get(VMContext, "false"));
>
> I think this should be:
>
>     getModule().addModuleFlag(llvm::Module::Error, "Disable Global Merge", 1u);

Akira also proposed "disable" ;)  I'll change that and the
string->unsigned, then!

-Ahmed

>> +}
>> +
>>  void CodeGenModule::EmitCoverageFile() {
>>    if (!getCodeGenOpts().CoverageFile.empty()) {
>>      if (llvm::NamedMDNode *CUNode = TheModule.getNamedMetadata("llvm.dbg.cu")) {
>