[PATCH] D28169: [ThinLTO] Subsume all importing checks into a single flag

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

In https://reviews.llvm.org/D28169#634361, @tejohnson wrote:

> In https://reviews.llvm.org/D28169#632825, @mehdi_amini wrote:
>
> > In https://reviews.llvm.org/D28169#632794, @tejohnson wrote:
> >
> > > In https://reviews.llvm.org/D28169#632559, @mehdi_amini wrote:
> > >
> > > > So we're losing some information for the ThinLink, which is somehow scary, I hope we won't have to backtrack later.
> > > >  Originally we had these separate flags because we expected to propagate more information across modules. But I don't see any use-case that would miss these flags right now, so...
> > >
> > >
> > > Thinking through the individual flags, I am somewhat questioning whether NoRename should be split out. Here's what this patch folds into NotEligibleToImport, along with some thoughts on whether we could eventually import them:
> > >
> > > 1. NoRename: can't import a def or a ref if local because that currently requires promotion/renaming. But what if in the future we would be able to import as a local copy? In that case it would be useful to split this out as a separate flag. But right now this is set when it is a local in a section - is it legal to create a copy in that case? If not, we would need to create a different flag for any non-renamable copyable cases that pop up in the future anyway.
> >
> >
> > It is likely that we'd need a separate flag to express the legality of the copy anyway.
> >  So it would be `NotEligibleForImport` (implicitly "as available_externally"), but another flag could say `CanBeDuplicated` (straw man name).
> >  The logic in the thin-link is likely gonna be easier this way (less special case or combination to check).
> >
> > That said we'll need to be careful about what we do with the promotion, I suspect we want to assert (I mean when assertions are enabled) that all the local selected for global promotion passes the existing `NoRename` test.
>
>
> Which test? The promotion logic in FunctionImportUtils (which happens in the backend) was already not able to assert on the NoRename flag when renaming on import since we may not have summaries for all references being imported (in the distributed backend case).


I meant that before we were verifying during the import that we don't import a function that references a function that can't be renamed, because we had this information. Now we don't, so the importer is not explicitly checking.
It's not critical, I guess was can trust the information to be correct, I was imagining that an assertion in the backend would help debugging "when things go wrong".

> Or are you suggesting adding back the NoRename flag to the summary

No, I was really imagining checking *on the IR* during the backend that what we asked the `backend to do during the ThinLink is not violating anything.

>>> 4. Anything that references a non-renamable/promoteable local (what I am moving from the thin link to the summary generation with this patch). If we can import these referenced locals as a copy (see #1 above), then we will simply need to stop setting the NotEligibleToImport flag on references to those copyable non-renamable locals.  Then once we decide to import any function, we'll need to walk through its references/calls and mark any such references for import (presumably the import logic will ensure that any local function marked for import that is also marked as non-renamable will be imported as a local copy).
>> 
>> I wonder if instead of stopping to set the `NotEligibleToImport` flag for such cases, we may be better to add another flag `CanBeImportedWithRefCopy`. That's because it'd avoid inspecting all the refs of all the global consider for  importing, and only do it when `CanBeImportedWithRefCopy` is set.
>> 
>>> So I don't think we need to split this one out into a separate flag.
> 
> It sounds like you are arguing for a separate flag (CanBeImportedWithRefCopy) just above? Or do you mean once we add the copy ability (i.e. for now just set NotEligibleToImport)?

Yes, that's what I meant.


https://reviews.llvm.org/D28169