[PATCH] D18100: Simplify Logic in IRMover

[Rafael Espíndola via llvm-commits]

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/20160319/256d5990/attachment.html>