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

[Quentin Colombet]

Hi Nick,

On May 16, 2014, at 3:59 PM, Jiangning Liu <liujiangning1 at gmail.com> wrote:

> 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.
Unless I am mistaken, this pass gives up on any global that uses a special alignment:
    // Ignore fancy-aligned globals for now.
    unsigned Alignment = DL->getPreferredAlignment(I);
    Type *Ty = I->getType()->getElementType();
    if (Alignment > DL->getABITypeAlignment(Ty))
      continue;

Thanks,
-Quentin
> 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
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140516/8c8ac432/attachment.html>