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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D28169#634564, @mehdi_amini wrote:

> 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.


Ah ok, that's more than what we do currently. Do you think I should add that to this patch?



================
Comment at: test/ThinLTO/X86/drop-debug-info.ll:5
-; The imported module has out-of-date debug information, let's make sure we can
-; drop them without crashing when materializing later.
 ; RUN: llvm-lto -thinlto-action=import %t.bc -thinlto-index=%t.index.bc -o - | llvm-dis -o - | FileCheck %s
----------------
tejohnson wrote:
> mehdi_amini wrote:
> > mehdi_amini wrote:
> > > tejohnson wrote:
> > > > mehdi_amini wrote:
> > > > > I'd be OK to drop this test
> > > > I'll go ahead and give it the same treatment you did to the other test (using llvm-link).
> > > That's not equivalent though: the other test was stressing bitcode loading, it could be done equally with llvm-link.
> > > This test was making sure we correctly "upgrade" the debug info before linking the IR in FunctionImport.cpp (IIRC). Changing to llvm-link won't exercise this code-path and we'll lose this coverage anyway.
> > > Thinking about it again, we may be able to do it with `opt -functionimport` though
> > Thinking more, the best would likely be to refactor `llvm-link` to call `FunctionImporter::importFunctions`. It is silly to have this custom logic in llvm-link.
> That sounds like a good change to make at some point, for now I will change this test to use opt -function-import though.
Actually, opt -function-import has the same issues as llvm-lto - it won't decide to import after this patch. I went ahead and reworked llvm-link to use the FunctionImporter, and changed this test to use it (D28277). I'll remove this change from the patch.


https://reviews.llvm.org/D28169