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

Mehdi AMINI via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 30 11:32:07 PST 2016


mehdi_amini added a comment.

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.
This is because now when a function `foo` references a local `bar`, but `bar` is marked as `NotEligibleForImport`, we still allow to import `foo` and `bar` will be promoted. We have no way during the thin-link to check that `bar` wasn't originally `NoRename`. 
Of course it "shouldn't" be a problem if the logic is right, but with us reading back bitcode with this flag, any fix to the detection of NoRename (or consistency) would be problematic.
(It may not be practical or easy to check this in the backend though).

> 3. IsNotViableToInline: This one is a hint, not correctness, and currently only set when the function is variadic. If the inliner is ever able to inline variadic functions, we could just remove this flag (or stop setting the new NotEligibleToImport combined flag in that case). So I don't think it is currently worthwhile to split this out into a separate flag anyway. If we want to use inliner information for more complex importing decisions in the thin link I think this would have to be made into a richer set of flags anyway. Or would we ever want to import something that isn't inlineable?

Till here I follow. And I don't see why we would import something that isn't inlinable.

> The only case I can think of is if it is also a local function that is non-renamable/promoteable, we want to import a reference to it, and we have the ability to import such non-renamable/promoteable local functions as local copies. In that case though, I would argue that we could simply stop marking these functions as NotEligibleToImport - it is just a small compile time/memory improvement anyway.

If it is also a function that is `NoRename `, then we'll apply the logic in 1) above, but this seems orthogonal.

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

Right.


https://reviews.llvm.org/D28169





More information about the llvm-commits mailing list