[llvm] r208934 - Implement global merge optimization for global variables.

[Rafael Espíndola]

>> +static cl::opt<bool> EnableGlobalMergeOnExternal(
>> +    "global-merge-on-external", cl::Hidden,
>> +    cl::desc("Enable global merge pass on external linkage"),
>> cl::init(false));
>> +
>>
>> This is a transitional option, right? I assume it will go away once it
>> is tested and benchmarked. Please add a comment stating that if that
>> is the case.
>
>
> I'm not sure it is a transitional option. It's harmless if we leave it
> there, right?

Not quiet. We don't like having too many switches. If this is
something that is likely to be permanent (why?) it might be better to
make it an option that is passed to the pass constructor.

> The global merge benefit comes from target requirement. For
> example, for a target with few registers, we may not want to enlarge base
> address live range, although this is probably the same case for the original
> static global merge. So it would be easily for us to switch on/off for
> different targets.
>
> However, I'm fine to add a comment like that if you want, and it doesn't
> hurt anything for me.

These are things that are better handled by a target switch. In this
case, since this is just an on/off thing, it can probably be made into
a pass constructor option and some targets can pass in true, other
pass in false.

For now this is only used for AArch64 and ARM, where the expected
value is always true, so maybe the best is to just leave the option
out?

If you need it for debugging/testing during development, that is fine,
but we should have a plan on how to remove it.

>>
>>
>> -INITIALIZE_PASS(GlobalMerge, "global-merge",
>> -                "Global Merge", false, false)
>>
>> +static void *initializeGlobalMergePassOnce(PassRegistry &Registry) {
>> +  PassInfo *PI = new PassInfo(
>> +      "Merge global variables", "global-merge", &GlobalMerge::ID,
>> +      PassInfo::NormalCtor_t(callDefaultCtor<GlobalMerge>), false, false,
>> +      PassInfo::TargetMachineCtor_t(callTargetMachineCtor<GlobalMerge>));
>> +  Registry.registerPass(*PI, true);
>> +  return PI;
>> +}
>> +
>> +void llvm::initializeGlobalMergePass(PassRegistry &Registry) {
>> +  CALL_ONCE_INITIALIZATION(initializeGlobalMergePassOnce)
>> +}
>>
>>
>> Why is this necessary?
>
>
> This is because we want to register global merge statically, then opt can
> work with this pass. Otherwise, we wouldn't be able to check the correctness
> of this middle-end pass with opt. This is also why I move some test cases
> from CodeGen to Transforms.

I guess the question is more, what is different from the existing
macros? Other passes seem to just use a macro and are usable from opt.
Looking a what the macro expands too, the difference is that you pass
the target machine ctor too. Please add a comment saying that is the
difference from what INITIALIZE_PASS would do.

If you are really motivated an ever better change would be to add a
INITIALIZE_TM_PASS macro :-)

On the new patches:

* 0001-Rename-global-merge-to-enable-global-merge.patch

Why do you need it to not be hidden? LGTM with it being hidden.

I think patch 0002 is OK too with a comment about why the init macro
is not used.

Cheers,
Rafael