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

[Jiangning Liu]

Hi Rafael,


2014-06-11 1:58 GMT+08:00 Rafael Espíndola <rafael.espindola at gmail.com>:

> >> +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.
>

The plan is to fix the so-called ADRP CSE issue in aarch64 back-end first,
and then we can go back to clean and this switch and improve this pass if
necessary.


>
> >>
> >>
> >> -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 :-)
>

Actually I'm not the one creating this new usage, for
example, CodeGenPreparePass is not using INITIALIZE_PASS directly either. I
just simply followed the current example code in trunk. We use a special
TargetMachineCtor just because we want to initialize TargetMachine, and
some TargetLowering stuff needs to be invoked in this pass, because it is
right now being invoked in PreISel stage.

But anyway, I think your proposal of creating INITIALIZE_TM_PASS is good.


> 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.
>

OK. Committed as r210639.


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

Committed as revision r210640.

Finally I also committed r210641 by creating a separate macro
INITIALIZE_TM_PASS, and three passes' initialization was simplified
accordingly.

Thanks,
-Jiangning


>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140611/a423fb45/attachment.html>