[PATCH] D18100: Simplify Logic in IRMover

[Mehdi Amini via llvm-commits]

So with your patch in D18252, this patch is OK?

-- 
Mehdi

> On Mar 17, 2016, at 1:25 PM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> 
> http://reviews.llvm.org/D18252 is the first step.
> 
> Cheers,
> Rafael
> 
> 
> On 17 March 2016 at 10:10, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>> The llvm-link answer is wrong. It is trying to copy both @a1. I will
>> see if that is simple to fix.
>> 
>> Cheers,
>> Rafael
>> 
>> 
>> On 16 March 2016 at 19:37, Mehdi Amini <mehdi.amini at apple.com> wrote:
>>> I tried running this with llvm-link instead of gold and it does not pass either:
>>> 
>>> llvm-link test/tools/gold/X86/comdat.ll  test/tools/gold/X86/Inputs/comdat.ll -S
>>> 
>>> The test expect the two comdat variables to be there, and only c2 is. Any hint on the discrepancy between gold and llvm-link?
>>> 
>>> Thanks,
>>> 
>>> --
>>> Mehdi
>>> 
>>>> On Mar 16, 2016, at 6:47 PM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>>>> 
>>>> This breaks tools/gold/X86/comdat.ll
>>>> 
>>>> 
>>>> On 15 March 2016 at 08:25, Teresa Johnson <tejohnson at google.com> wrote:
>>>>> tejohnson added a comment.
>>>>> 
>>>>> I applied this patch and successfully built SPEC cpu2006 with both full and thin LTO. However, I'd like to wait until Rafael takes a look since he added this as part of his patch to split the linker in 2.
>>>>> 
>>>>> Tracing through the code this morning, I where this change will make a difference in how the mapping works. Because we use a separate map when linking an aliasee definition, and also use a different materializer LValMaterialize of type LocalValueMaterializer, the ForAlias flag gets set to true. If you look at IRLinker::linkGlobalValueProto you can see where this will make a difference. Since we are using a different value map, it seems like we should be trying to materialize GVs that may even have been mapped already (in the ValueMap but not the AliasValueMap). Then when we call back into linkGlobalValueProto when mapping the Aliasee body for these encountered GVs, ForAlias will be true. Here are some relevant snippets from linkGlobalValueProto, with my own comments added:
>>>>> 
>>>>> // Teresa: this will essentially force the subsequent code to create a NewGV for this GV
>>>>> if (!ShouldLink && ForAlias)
>>>>>   DGV = nullptr;
>>>>> 
>>>>> <snipped the mapping code>
>>>>> 
>>>>> // Teresa: This will cause the NewGV to be placed in a COMDAT if necessary
>>>>> if (ShouldLink || ForAlias) {
>>>>>   if (const Comdat *SC = SGV->getComdat()) {
>>>>>     if (auto *GO = dyn_cast<GlobalObject>(NewGV)) {
>>>>>       Comdat *DC = DstM.getOrInsertComdat(SC->getName());
>>>>>       DC->setSelectionKind(SC->getSelectionKind());
>>>>>       GO->setComdat(DC);
>>>>>     }
>>>>>   }
>>>>> }
>>>>> 
>>>>> // Teresa: Here is a key difference - GV is mapped (possibly after already being mapped by due to a non-aliasee reference), and the new copy is internal.
>>>>> if (!ShouldLink && ForAlias)
>>>>>   NewGV->setLinkage(GlobalValue::InternalLinkage);
>>>>> 
>>>>> I vaguely recall Rafael discussing linking aliasees as internal - aha, see the discussion relating to aliasees and comdats in http://reviews.llvm.org/D14623. I believe this change was to implement his suggested fix. Also see test/Linker/alias.ll which was modified to check this as part of the linker splitting change. However, that is still passing with this patch applied...
>>>>> 
>>>>> As an aside, if this change is ok and accepted by Rafael, the LocalValueMaterializer and instances of ForAlias should also be removed as they would be dead.
>>>>> 
>>>>> 
>>>>> http://reviews.llvm.org/D18100
>>>>> 
>>>>> 
>>>>> 
>>>