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

Jiangning Liu liujiangning1 at gmail.com
Thu Jun 5 19:51:30 PDT 2014


Hi Rafael,


2014-06-05 22:01 GMT+08:00 Rafael EspĂ­ndola <rafael.espindola at gmail.com>:

> On 5 June 2014 03:26, Jiangning Liu <liujiangning1 at gmail.com> wrote:
> > Hi Rafael,
> >
> > I think now I understand your point and agree with you this alignment
> setup
> > can be treated as a pure back-end issue AArch64 can handle it privately
> and
> > we can avoid exposing it to shared code.
> >
> > So now attached is the updated patch by removing -global-merge-aligned
> > switch. I will work out separate patches to handle AArch64 specific stuff
> > later on.
>
> Awesome!
>
> -static cl::opt<bool>
> -EnableGlobalMerge("global-merge", cl::Hidden,
> -                  cl::desc("Enable global merge pass"),
> -                  cl::init(true));
> +static cl::opt<bool> EnableGlobalMerge("enable-global-merge",
> cl::NotHidden,
> +                                       cl::desc("Enable global merge
> pass"),
> +                                       cl::init(true));
>
> Please commit this option rename first to reduce the noise in the patch.
>

This renaming is because the global option would be conflict with global
pass enabling name otherwise, so the renaming itself can't justify why it
needs to be changed,
but I'm fine to get it split from current patch if you want.


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


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


>
> +  initializeGlobalMergePass(Registry);
>
> same here.
>
>
> +attributes #0 = { nounwind ssp "less-precise-fpmad"="false"
> "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"
> "no-infs-fp-math"="false" "no-nans-fp-math"="false"
> "stack-protector-buffer-size"="8" "unsafe-fp-math"="false"
> "use-soft-float"="false" }
> +attributes #1 = { "less-precise-fpmad"="false"
> "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"
> "no-infs-fp-math"="false" "no-nans-fp-math"="false"
> "stack-protector-buffer-size"="8" "unsafe-fp-math"="false"
> "use-soft-float"="false" }
> +attributes #2 = { nounwind readnone ssp "less-precise-fpmad"="false"
> "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"
> "no-infs-fp-math"="false" "no-nans-fp-math"="false"
> "stack-protector-buffer-size"="8" "unsafe-fp-math"="false"
> "use-soft-float"="false" }
> +attributes #3 = { nounwind }
> +
> +!llvm.ident = !{!0}
> +
> +!0 = metadata !{metadata !"LLVM version 3.4 "}
> +!1 = metadata !{metadata !2, metadata !2, i64 0}
> +!2 = metadata !{metadata !"int", metadata !3, i64 0}
> +!3 = metadata !{metadata !"omnipotent char", metadata !4, i64 0}
> +!4 = metadata !{metadata !"Simple C/C++ TBAA"}
>
> You probably don't need all this in the test.
>

This is not introduced by my patch, but I'm OK to remove them with this
patch if you think it's necessary.

Thanks,
-Jiangning


>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140606/e4273cb4/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Global-merge-for-global-symbols.patch
Type: text/x-patch
Size: 28138 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140606/e4273cb4/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Rename-global-merge-to-enable-global-merge.patch
Type: text/x-patch
Size: 3831 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140606/e4273cb4/attachment-0001.bin>


More information about the llvm-commits mailing list