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

[Ahmed Bougacha]

On Mon, Mar 2, 2015 at 1:15 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:
> 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!

We discussed this a bit more, and we agreed on a "Global Merge" flag,
following Duncan's logic:  the disabled case (either -mno-global-merge
or -O < 3) adds an "Override" flag, vs. "Error" for the enabled case.
The default is to enable it, when the flag isn't present, to match the
current behavior.  Eric, thoughts?

-Ahmed