<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div apple-content-edited="true">Hi,
</div>
<br><div><div>On May 17, 2014, at 6:15 AM, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><blockquote type="cite">I don't think Nick is understanding this issue correctly as I mentioned in<br>my last reply, so I'm not sure if you are really agree with him. The issue<br>you mentioned below is actually completely different from Nick's point.<br>Refer to my answer below, anyway.<br></blockquote><br>The part about unnamed_addr, yes, but not the part about alignment,<br>right? Things like getGlobalMergeAlignment do look out of place.<br></blockquote><div>Again, but looks like my reply did not make it to the list, we give 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><br><blockquote type="cite"><br><blockquote type="cite"><blockquote type="cite">Is<br>this uncommon enough that checking for uses coming from the same<br>function was not considered profitable?<br></blockquote><br><br>I'm not sure if this could be profitable, because they are global variables<br>and we don't really know how they are being referenced in other modules. The<br>heuristic of considering the uses within a function for current module would<br>not probably give the same benefit to other modules, so I'm thinking the<br>heuristic strategy of merging them all together for current module would not<br>make big difference from the one of further checking uses within a function<br>for current module.<br><br>I never say my optimization patch is commonly profitable, and this is why<br>they are under switches control. But for some specific cases, it would be<br>profitable, I believe In particular, for the case that global variables are<br>being heavily used in the module that are defining them.<br></blockquote><br>Well, we normally don't want to have too many use facing switches.<br>What is the plan for removing this one? If variable concatenation (to<br>avoid confusion with merging which causes the address to be the same)<br>is profitable enough for ARM/AARCH64 that it is better than the<br>inability to dead strip some code, then it should always be enabled.<br>If not, we should figure out an heuristic for when to do it.<br><br>It should also be done really late. In particular, it should probably<br>not run with -flto and instead be enabled in the LTO pipeline.<br></blockquote>I agree with the LTO thing.</div><div>That said, this pass cannot and is not run late in the pipeline. Basically, it changes the code during the pass initialization (pretty broken I know!), but this is currently mandatory otherwise the debug information are wrong:</div><div><a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130415/171763.html">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130415/171763.html</a></div><div><br><blockquote type="cite"><br>I also don't follow your objection to looking at the function bodies<br>to decide when it is profitable. If a variable is external, codegen<br>must produce the fully general access pattern. That is, the only code<br>that benefits from concatenating global variables is the one in the<br>same TU as the variables, right?<br></blockquote><div>I agree, but I point out that this pass is currently very cheap (in term of compile time) because it has a very naive heuristic (merge variable per size). If we want to change that, this is likely that we will break the nice compile time behavior. I am not opposed to that, this is just something we must be aware of.</div><div>If we want to do something more clever, it is likely what we end up with a coalescing problem with all the issues this kind of solution may have.</div><div><br></div>Anyhow, whatever we decide to do, we must carefully benchmark it.</div><div><br></div><div>Thanks,</div><div>-Quentin<br><blockquote type="cite"><br><blockquote type="cite"><blockquote type="cite">putting two variables that a function uses next to each other would<br>also be a win there, no?<br></blockquote><br><br>X86 and ARM/AArch64 have different address model, and this is because of the<br>so-called CSIC and RISC differences. X86 can directly encode global address<br>relocation into instructions, while ARM/AArch64 can only encode it in<br>load/store instructions. For AArch64, this is getting worse, because ADRP<br>instruction can only get the page address of global variable due to the<br>limitation of instruction encoding size (32-bit only), and we have to<br>introduce another add instruction to get the 64-bit address. This is why<br>this optimization is very meaningful to AArch64. I think you would be able<br>to understand this much better if you look into the very beginning part of<br>my patch review thread.<br><br>Yes, putting two global variables that a function next each other would be a<br>win, but as I mentioned above this is also heuristic because we don't know<br>how global variables are being used by other modules at compilation time.<br></blockquote><br>It is heuristic. I was just pointing out that it is not fundamentally<br>an ARM only thing.<br><br><blockquote type="cite"><br>OK. Thanks for letting me know that. Hopefully my patch still can reuse your<br>new GlobalAlias solution. Looking at the bug tracker you pointed to me, it<br>seems to be a very old problem raised by Christ long time ago. I'm not sure<br>if you can really make it happen next week. I would be extremely<br>appreciative if you do.<br></blockquote><br>I think I can. The main change is already in. What is missing is just<br>the adding offsets.<br><br>Cheers,<br>Rafael<br>_______________________________________________<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></body></html>