<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hi Nick,<div><br><div><div>On May 16, 2014, at 3:59 PM, Jiangning Liu <<a href="mailto:liujiangning1@gmail.com">liujiangning1@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">Hi Nick,<div><br></div><div>Thanks for your kindly review!</div><div><br></div><div>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.</div>
<div><br></div><div>For GlobalValue with specific alignment attribute like __attribute__ ((aligned(16))), I believe the following code could skip it,</div><div><br></div><div><div>322 // Ignore fancy-aligned globals for now.</div>
<div>323 unsigned Alignment = DL->getPreferredAlignment(I);</div><div>324 Type *Ty = I->getType()->getElementType();</div><div>325 if (Alignment > DL->getABITypeAlignment(Ty))</div><div>326 continue;</div>
</div><div><br></div><div>I think it should work for all kinds of global values, and I didn't change it at all.</div><div><br></div><div>Thanks,</div><div>-Jiangning<br><br>Nick Lewycky <<a>nicholas@mxc.ca</a>>于2014年5月17日星期六写道：<br>
<blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto;">Jiangning Liu wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Author: jiangning<br>
Date: Thu May 15 18:45:42 2014<br>
New Revision: 208934<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=208934&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=208934&view=rev</a><br>
Log:<br>
Implement global merge optimization for global variables.<br>
<br>
This commit implements two command line switches -global-merge-on-external<br>
and -global-merge-aligned, and both of them are false by default, so this<br>
optimization is disabled by default for all targets.<br>
</blockquote>
<br>
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.<br>
<br>
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.<br>
<br>
Also, I'm concerned about this:<br>
<br>
+ virtual unsigned getGlobalMergeAlignment(<u></u>StructType *MergedTy) const {<br>
+ return getDataLayout()-><u></u>getABITypeAlignment(MergedTy);<br>
+ }<br>
<br>
You don't know that a global only requires ABI alignment, nor whether it could tolerate less than ABI alignment.</blockquote></div></div></blockquote><div>Unless I am mistaken, this pass gives up on any global that uses a special alignment:</div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"> // Ignore fancy-aligned globals for now.</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"> unsigned Alignment = DL->getPreferredAlignment(I);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"> Type *Ty = I->getType()->getElementType();</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"> if (Alignment > DL->getABITypeAlignment(Ty))</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"> continue;</div></div><div><br></div><div>Thanks,</div>-Quentin<br><blockquote type="cite"><div dir="ltr"><div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto;"> 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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
Does that make sense? Sorry if this has already been debated on the mailing list previously and I simply missed it.<br>
<br>
Nick<br>
</blockquote></div>
</div>
_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote></div><br></div></body></html>