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

[Jiangning Liu]

Hi Nick,

Thanks for your kindly review!

I think unnamed_addr attribute only means we don't really care the address
of a global value. This optimization doesn't result in &glb1 == &glb2 at
all. The merge is not to simply generate aliases for those those global
values, but creating a newly merged global value to cover all of them, and
essentially it intends to re-arrange the global memory layout only. If
originally the address of the original global value is significant, the
optimization will still keep it as before.

For GlobalValue with specific alignment attribute like __attribute__
((aligned(16))), I believe the following code could skip it,

322     // Ignore fancy-aligned globals for now.
323     unsigned Alignment = DL->getPreferredAlignment(I);
324     Type *Ty = I->getType()->getElementType();
325     if (Alignment > DL->getABITypeAlignment(Ty))
326       continue;

I think it should work for all kinds of global values, and I didn't change
it at all.

Thanks,
-Jiangning

Nick Lewycky <nicholas at mxc.ca>于2014年5月17日星期六写道：

> Jiangning Liu wrote:
>
>> Author: jiangning
>> Date: Thu May 15 18:45:42 2014
>> New Revision: 208934
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=208934&view=rev
>> Log:
>> Implement global merge optimization for global variables.
>>
>> This commit implements two command line switches -global-merge-on-external
>> and -global-merge-aligned, and both of them are false by default, so this
>> optimization is disabled by default for all targets.
>>
>
> I think this entire optimization has the wrong design, and I think I'd
> like you to revert this patch -- unless I'm mistaken of course. Let me
> explain why and you can decide to revert.
>
> Firstly, there shouldn't be any switches to control global merging. If
> it's legal to merge, the global will be marked with "unnamed_addr", which
> means that the address of the global is itself not relevant. If you don't
> have this, you may not merge since "&glbl1 == &glbl2" must be correct. Your
> test shows you merging two globals which don't have unnamed_addr. That is a
> miscompile.
>
> Also, I'm concerned about this:
>
> +  virtual unsigned getGlobalMergeAlignment(StructType *MergedTy) const {
> +    return getDataLayout()->getABITypeAlignment(MergedTy);
> +  }
>
> You don't know that a global only requires ABI alignment, nor whether it
> could tolerate less than ABI alignment. There's an "alignment" value on the
> global value itself, you need to rely on that. A 'char
> __attribute__((aligned(16)))' needs to have 16-byte alignment after
> merging, not the i8's usual 1-byte alignment.
>
> The correct approach is to rely on unnamed_addr and alignment, and work on
> the optimizers and frontends to add unnamed_addr where possible and to
> adjust alignment where needed.
>
> You can even have a mode for your frontend, a -fmerge-all-globals flag
> which slaps unnamed_addr on literally everything. It won't be a conforming
> C implementation that way, but at least LLVM will be told it's okay to
> merge before doing the merging.
>
> Does that make sense? Sorry if this has already been debated on the
> mailing list previously and I simply missed it.
>
> Nick
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140517/ea83cf69/attachment.html>