[PATCH] D42528: [LTO] - Introduce GlobalResolution::Prevailing flag.

Teresa Johnson via llvm-commits llvm-commits at lists.llvm.org
Thu Jan 25 07:47:21 PST 2018


On Thu, Jan 25, 2018 at 7:30 AM, George Rimar <grimar at accesssoftek.com>
wrote:

> Sorry, I thought I am responding to D42107 thread :)
> In this thread you right. I'll update the patch in a minute.
>

I assume in D42107 you will change it back to what this patch originally
did then (always setting IRName)? To me it is essentially NFC either way, I
don't have an issue with the original version of this patch, but will let
you and Rafael decide.
(LGTM in any case, although one suggestion is to add a comment to the
declaration of the isPrevailing bool field (i.e. that IR contains the
prevailing definition).
Teresa


> Best regards,
> George | Developer | Access Softek, Inc
>
> ________________________________________
> От: George Rimar
> Отправлено: 25 января 2018 г. 18:20
> Кому: Rafael Avila de Espindola; reviews+D42528+public+
> 221bfc9b60e2bf03 at reviews.llvm.org; tejohnson at google.com
> Копия: joker.eph at gmail.com; llvm at inglorion.net; Igor Kudrin; Evgeny
> Leviant; llvm-commits at lists.llvm.org
> Тема: Re: [PATCH] D42528: [LTO] - Introduce GlobalResolution::Prevailing
> flag.
>
> >> Index: lib/LTO/LTO.cpp
> >> ===================================================================
> >> --- lib/LTO/LTO.cpp
> >> +++ lib/LTO/LTO.cpp
> >> @@ -419,11 +419,14 @@
> >>      auto &GlobalRes = GlobalResolutions[Sym.getName()];
> >>      GlobalRes.UnnamedAddr &= Sym.isUnnamedAddr();
> >>      if (Res.Prevailing) {
> >> -      assert(GlobalRes.IRName.empty() &&
> >> +      assert(!GlobalRes.Prevailing &&
> >>               "Multiple prevailing defs are not allowed");
> >> -      GlobalRes.IRName = Sym.getIRName();
> >> +      GlobalRes.Prevailing = true;
> >>      }
> >>
> >> +    if (GlobalRes.IRName.empty())
> >> +      GlobalRes.IRName = Sym.getIRName();
> >
> >Why do you need the second if? It is still true that only for prevailing
> >symbols we need the name, no?
>
> No. Previously GlobalRes.IRName was set only for prevailing symbols.
> And so "if (GlobalRes.IRName)" was used like "if (Prevailing)".
>
> But now I need IRName for both privailing and not-prevailing symbols
> (see change in Error LTO::run(AddStreamFn AddStream, NativeObjectCache
> Cache) ).
> So I had to add bool Prevailing flag.
>
> With that I can mark Live all PrevailingType::Yes and
> PrevailingType::Unknown
> and make PrevailingType::No dead.
>
> >That is, could this be:
> >
> >  if (Res.Prevailing) {
>  >     assert(!GlobalRes.Prevailing &&
>  >            "Multiple prevailing defs are not allowed");
>  >     GlobalRes.IRName = Sym.getIRName();
>  >     GlobalRes.Prevailing = true;
>  >     GlobalRes.IRName = Sym.getIRName();
> >  }
> >
> >Cheers,
> >Rafael
>
> George.
>



-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180125/2a85559a/attachment.html>


More information about the llvm-commits mailing list