[PATCH] D31443: [LTO] Do not reorder global variables unnecessarily during merging

[Mehdi Amini via llvm-commits]

> On Mar 29, 2017, at 11:20 AM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> 
> On 28 March 2017 at 18:29, Gerolf Hoflehner via Phabricator
> <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
>> Gerolf added a comment.
>> 
>> I'd love to see some comments an get your thoughts about a verifier.
>> 
>> Thank you
>> Gerolf
>> 
>> 
>> 
>> ================
>> Comment at: lib/Object/ModuleSymbolTable.cpp:48
>>     SymTab.push_back(&GV);
>> +  for (Function &F : *M)
>> +    SymTab.push_back(&F);
>> ----------------
>> Please add a comment about why the order of the for loop is relevant and how it relates to source code ordering. Also, could there be a verifier that source code order is preserved?
>> 
>> 
>> ================
>> Comment at: test/LTO/Resolution/X86/globalorder.ll:1
>> +; Check that the order of global variables is not unnecessarily
>> +; perturbed by LTO during module merging.
>> ----------------
>> How about 'Check that LTO keeps global variables in source code order'?
> 
> No. That is not something we want to do.
> 
> In fact, I am a bit opposed to this change just because it is setting
> the wrong precedence. The output is not defined to be in any specified
> order.

I sympathize with this, and I’m OK with this change only because it is going from a random order to another, with a 3 lines change. Anyone having a reason to change it to another order (i.e. not random) should be free to do so and not be locked by this change.

> Anything trying to optimize the order should operate at a level
> lower than llvm IR.

I think an LTO IR Pass that operate at the IR level should be OK as well. What’s wrong with that? It’s much more friendly than lower level (binary…) and better integrated with things like PGO. Also it makes the optimization independent of the linker.
I’m not saying it is wrong to do it at the linker level as well, but I don’t see a problem with such an IR pass.

Cheers,

— 
Mehdi

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170329/9f79a976/attachment.html>