[PATCH] D18100: Simplify Logic in IRMover

[Rafael Espíndola via llvm-commits]

Correct. The fist patch is just yak shaving  on the way to fix llvm-link.
On Mar 19, 2016 4:50 PM, "Mehdi Amini" <mehdi.amini at apple.com> wrote:

> This is what I thought.
> However your patch does not seem to affect how llvm-link handles
> test/tools/gold/X86/comdat.ll? (still fails).
>
> --
> Mehdi
>
>
>
>
> On Mar 19, 2016, at 11:46 AM, Rafael Espíndola <rafael.espindola at gmail.com>
> wrote:
>
> No, sorry. This patch is wrong. My patch is the first step in exposing the
> problem with llvm-link.
>
> Cheers,
> Rafael
> On Mar 19, 2016 1:11 AM, "Mehdi Amini" <mehdi.amini at apple.com> wrote:
>
>> 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
>> >>>>>
>> >>>>>
>> >>>>>
>> >>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160320/1ece0202/attachment.html>