[PATCH] D18100: Simplify Logic in IRMover

[Mehdi Amini via llvm-commits]

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
>> 
>> 
>>