[LLVMdev] RFC: ThinLTO Impementation Plan

[David Blaikie]

On Fri, May 15, 2015 at 9:53 AM, Teresa Johnson <tejohnson at google.com>
wrote:

> On Fri, May 15, 2015 at 9:20 AM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> > On Fri, May 15, 2015 at 9:18 AM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>>
> >>>
> >>> >> - Marking of imported functions (for use in ThinLTO-specific symbol
> >>> >> linking and global DCE, for example).
> >>> >
> >>> > Marking how?  Do you mean giving them internal linkage, or something
> >>> > else?
> >>>
> >>> Mentioned just after this: either an in-memory flag on the Function
> >>> class, or potentially in the IR. For the prototype I just had a flag
> >>> on the Function class.
> >>
> >>
> >> Would this be anything other than "available externally" linkage?
> (either
> >> this module is responsible for emitting the definition of this function
> for
> >> other modules to call, or it's not - if it's not, either this module
> depends
> >> on another module to provide a definition (this would be "available
> >> externally") or it doesn't (this would be "internal" linkage")) - I
> think...
> >> though I'm sort of jumping into the middle of some of this & might've
> missed
> >> some context, sorry.
> >
> >
> > & I see your follow up further down in that thread:
> >
> > "There were only a couple minor tweaks required here (under the flag I
> > added to the Function indicating that this was imported). Only
> > promoted statics are remarked available_externally. For a
> > non-discardable symbol that was imported, we can discard here since we
> > are done with inlining (it is non-discardable in its home module)."
> >
> > &, like Duncan, I'll wait for more details on that front. (may or may
> not be
> > useful to split some of these subthreads into separate email threads to
> keep
> > discussion clear - but I'm not sure)
>
> I just went back and looked at my prototype and I had remembered this
> wrong. An imported function is always marked
> AvailableExternallyLinkage, unless it has link once linkage.
>
> As far as using that to indicate that it is an aux function, I was
> concerned about overloading the meaning of that linkage type. See the
> next para for an example where I am unsure about doing this...
>
> Looking back through my GlobalDCE changes, it looks like one of the
> places I had changed (where we mark defined globals in runOnModule)
> already has a guard for !hasAvailableExternallyLinkage and
> !isDiscardableIfUnused, so my additional guard against marking
> imported functions is unnecessary. But the other place I had to change
> was in GlobalIsNeeded where it walks through the function and
> recursively marks any referenced global as needed. Here there was no
> guard against marking a global that is available externally as needed
> if it is referenced. I had added a check here to not mark imported
> functions as needed on reference unless they were discardable (i.e.
> link once). Is this a bug - should this have a guard against marking
> available externally function refs as needed?
>

Duncan's probably got a better idea of what the right answer is here. I
suspect "yes".

The trick with available_externally is to ensure we keep these around for
long enough that their definitions are useful (for inlining, constant prop,
all that good stuff) but remove them before we actually do too much work on
them, or at least before we emit them into the final object file.

I imagine if GlobalDCE isn't removing available_externally functions it's
because they're still useful in the optimization pipeline and something
further down the pipe removes them (because they do, ultimately, get
removed). Any idea what problematic behavior you were seeing that was
addressed by changing DCE to remove these functions was? I imagine whatever
the bad behavior was, it could be demonstrated without ThinLTO and with a
normal module with an available_externally function & would be worth
discussing what the right behavior is in that context, regardless of
ThinLTO.


>
> There was one other change to GlobalDCE that I had to make, where we
> erase the list of DeadFunctions from the module. In my case we may
> have a function body that was eliminated, but still have references to
> it (i.e. in the case I am talking about above). In that case it is now
> just a declaration and we don't erase it from the function list on the
> module. If the GlobalsIsNeeded code is changed to avoid marking
> available externally function refs as needed then this change would be
> needed for that case as well.
>
> Thanks,
> Teresa
>
> >
> >>
> >>
> >>
> >> - David
> >
> >
>
>
>
> --
> 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-dev/attachments/20150515/d67c472e/attachment.html>