Hi Rafael,<br><br>Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>>于2014年5月17日星期六写道：<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> I think this entire optimization has the wrong design, and I think I'd like<br>
> you to revert this patch -- unless I'm mistaken of course. Let me explain<br>
> why and you can decide to revert.<br>
<br>
I reverted because of its use o GEPs with non-zero offsets, but now<br>
that I read the patch I agree with Nick's concerns.<br></blockquote><div><br></div><div>I don't think Nick is understanding this issue correctly as I mentioned in my last reply, so I'm not sure if you are really agree with him. The issue you mentioned below is actually completely different from Nick's point. Refer to my answer below, anyway. </div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I also a bit confused by what is the advantage. It looks like the<br>
objective is to turn two global variables into a single one and have<br>
an alias that provides a way to get the old name.<br>
<br>
This then lets codegen use the address difference between the two<br>
produce better code in a function that accesses both variables.<br>
<br></blockquote><div><br></div><div>Yes. Your understanding is correct.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
But the code does not check if there is any function accessing both<br>
variables that are being merged. In fact, it can merge in a variable<br>
that is not used at all in the current module, right?<br></blockquote><div><br></div><div>Right. It's possible.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
The problem with that is that it will prevent the linker (even with<br>
LTO, unless it gets smarter) from dead stripping this variable. </blockquote><div><br></div><div>Yes. This might be the side effect.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Is<br>
this uncommon enough that checking for uses coming from the same<br>
function was not considered profitable?<br></blockquote><div><br></div><div>I'm not sure if this could be profitable, because they are global variables and we don't really know how they are being referenced in other modules. The heuristic of considering the uses within a function for current module would not probably give the same benefit to other modules, so I'm thinking the heuristic strategy of merging them all together for current module would not make big difference from the one of further checking uses within a function for current module.</div>
<div><br></div><div>I never say my optimization patch is commonly profitable, and this is why they are under switches control. But for some specific cases, it would be profitable, I believe In particular, for the case that global variables are being heavily used in the module that are defining them. </div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also, I know your change was not responsible for it, but why is this<br>
ARM or AARCH64 specific?It might not be as big a win on X86, but<br>
putting two variables that a function uses next to each other would<br>
also be a win there, no?<br></blockquote><div><br></div><div>X86 and ARM/AArch64 have different address model, and this is because of the so-called CSIC and RISC differences. X86 can directly encode global address relocation into instructions, while ARM/AArch64 can only encode it in load/store instructions. For AArch64, this is getting worse, because ADRP instruction can only get the page address of global variable due to the limitation of instruction encoding size (32-bit only), and we have to introduce another add instruction to get the 64-bit address. This is why this optimization is very meaningful to AArch64. I think you would be able to understand this much better if you look into the very beginning part of my patch review thread.</div>
<div><br></div><div>Yes, putting two global variables that a function next each other would be a win, but as I mentioned above this is also heuristic because we don't know how global variables are being used by other modules at compilation time.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
In any case, I hope to have a patch adding an explicit offset to<br>
GlobalAlias out this weekend or very early next week.<br></blockquote><div><br></div><div>OK. Thanks for letting me know that. Hopefully my patch still can reuse your new GlobalAlias solution. Looking at the bug tracker you pointed to me, it seems to be a very old problem raised by Christ long time ago. I'm not sure if you can really make it happen next week. I would be extremely appreciative if you do.</div>
<div> </div><div>Thanks,</div><div>-Jiangning</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks,<br>
Rafael<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="javascript:;" onclick="_e(event, 'cvml', 'llvm-commits@cs.uiuc.edu')">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote>