[ThinLTO] Subsume all importing checks into a single flag

[tejohnson]

Summary

This adds a new summary flag NotEligibleToImport that subsumes
several existing flags (NoRename, HasInlineAsmMaybeReferencingInternal
and IsNotViableToInline). It also subsumes the checking of references
on the summary that was being done during the thin link by
eligibleForImport() for each candidate. It is much more efficient to
do that checking once during the per-module summary build and record
it in the summary.

I wonder whether test/ThinLTO/X86/autoupgrade.ll and
test/ThinLTO/X86/drop-debug-info.ll should be removed, since we
now must conservatively not import anything defined by an old
version of the bitcode?

[mehdi_amini]

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

include/llvm/IR/ModuleSummaryIndex.h

125 ↗

(On Diff #82706)

Seems to me that it is NotEligibleToImport.

127 ↗

(On Diff #82706)

Delegate to the other Ctor:

GVFlags(const GlobalValue &GV, bool NotEligibleToImport)
    : GVFlags(GV.getLinkage(), NotEligibleToImport) {
...

lib/Analysis/ModuleSummaryAnalysis.cpp

191 ↗

(On Diff #82706)

This can be moved a few lines above when creating Flags IIUC.

244 ↗

(On Diff #82706)

I'm not sure if NonExternallyReferenceable is the best name, I feel it would be more accurate with CantBePromoted or something like that.
(Any local symbols is "not externally referenceable" in the absolute).

test/ThinLTO/X86/autoupgrade.ll

11 ↗

(On Diff #82706)

This was an annoying an subtle bug, I'm concerned about losing coverage for this.
If there is no other way, we should regenerate the bitcode file with the new summary.
I'm wondering now if this test can't be triggered with llvm-link instead of llvm-lto. I'll try and report.

test/ThinLTO/X86/drop-debug-info.ll

5 ↗

(On Diff #82706)

I'd be OK to drop this test

[mehdi_amini]

test/ThinLTO/X86/autoupgrade.ll

11 ↗

(On Diff #82706)

r290736

[mehdi_amini]

test/ThinLTO/X86/autoupgrade.ll

11 ↗

(On Diff #82706)

(and r290740)

[tejohnson]

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

2. HasInlineAsmMaybeReferencingInternal: The only way we can ever safely import something that has opaque references (via inline asm) is if we also imported all locals in the llvm.*used sets as local copies, just to be safe. Not sure we would ever want to do that - I suppose we could eventually encounter a function that contains inline asm and locals in the used set that is profitable to import/inline, but do we want to carry around an extra bit in the summary before we find such a case?

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

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). So I don't think we need to split this one out into a separate flag.

Thoughts?

include/llvm/IR/ModuleSummaryIndex.h

125 ↗

(On Diff #82706)

Here I specifically meant that the parameter indicates it is a local that can't be renamed/promoted - it is only one of the things that goes into determining NotEligibleToImport. Rethinking - I will move the variadic check into the caller (it seems more appropriate to have this logic where we compute the summary index anyway), then I will just nuke this constructor and have it invoke the above one.

127 ↗

(On Diff #82706)

Going to remove this one (see above comment)

lib/Analysis/ModuleSummaryAnalysis.cpp

191 ↗

(On Diff #82706)

I assume you mean the "if (NotRenamableLocal) ... " handling (in phab it looks like it's on the lines above). Yes, I can move this.

244 ↗

(On Diff #82706)

Ok, I can change the name. I was trying to find a name that indicates it is 1) a local (currently) and 2) can't be renamed/promoted. I think CantBePromoted is sufficiently descriptive.

test/ThinLTO/X86/autoupgrade.ll

11 ↗

(On Diff #82706)

Great thanks, forgot about that functionality.

test/ThinLTO/X86/drop-debug-info.ll

5 ↗

(On Diff #82706)

I'll go ahead and give it the same treatment you did to the other test (using llvm-link).

[mehdi_amini]

lib/Analysis/ModuleSummaryAnalysis.cpp

191 ↗

(On Diff #82706)

I meant FuncSummary->setNotEligibleToImport();.

IIUC it could be:

bool NonRenamableLocal = isNonRenamableLocal(F);
bool IsEligibleForImport = NonRenamableLocal || HasInlineAsmMaybeReferencingInternal
GlobalValueSummary::GVFlags Flags(F, IsEligibleForImport);

test/ThinLTO/X86/drop-debug-info.ll

5 ↗

(On Diff #82706)

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

[mehdi_amini]

In D28169#632794, @tejohnson wrote:

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

[mehdi_amini]

test/ThinLTO/X86/drop-debug-info.ll

5 ↗

(On Diff #82706)

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.

[tejohnson]

In D28169#632825, @mehdi_amini wrote:

In D28169#632794, @tejohnson wrote:

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

Or are you suggesting adding back the NoRename flag to the summary, and adding a check in the thin link (presumably after we decide on imports, to check that no imported ref is a local marked NoRename)?

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.

Only import as a copy for a reference we want to import/inline, as described below.

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.

Right (i.e. mark with a new "CanBeDuplicated" flag).

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.

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)?

test/ThinLTO/X86/drop-debug-info.ll

5 ↗

(On Diff #82706)

That sounds like a good change to make at some point, for now I will change this test to use opt -function-import though.

[mehdi_amini]

In D28169#634361, @tejohnson wrote:

In D28169#632825, @mehdi_amini wrote:

In D28169#632794, @tejohnson wrote:

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

[tejohnson]

In D28169#634564, @mehdi_amini wrote:

In D28169#634361, @tejohnson wrote:

In D28169#632825, @mehdi_amini wrote:

In D28169#632794, @tejohnson wrote:

In 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?

test/ThinLTO/X86/drop-debug-info.ll

5 ↗

(On Diff #82706)

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.

[mehdi_amini]

In D28169#634938, @tejohnson wrote:

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

Not necessarily: it'd be nice to have such validation of the expected invariant, but I have no idea how much work it is, so if you think you can do it in ~30min, I'd say "yes please", otherwise I would't require it for us to move forward. Sounds reasonable?

[tejohnson]

In D28169#634946, @mehdi_amini wrote:

In D28169#634938, @tejohnson wrote:

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

Not necessarily: it'd be nice to have such validation of the expected invariant, but I have no idea how much work it is, so if you think you can do it in ~30min, I'd say "yes please", otherwise I would't require it for us to move forward. Sounds reasonable?

I went ahead and added this in the new patch I am uploading.

include/llvm/IR/ModuleSummaryIndex.h

125 ↗

(On Diff #82706)

Removed this constructor, moved variadic check to module summary builder.

127 ↗

(On Diff #82706)

ditto

lib/Analysis/ModuleSummaryAnalysis.cpp

191 ↗

(On Diff #82706)

Done (but with variable being NotEligibleToImport not IsEligibleToImport)

244 ↗

(On Diff #82706)

done

[tejohnson]

Address review comments

[mehdi_amini]

LGTM, but have you checked that the number of imports is unchanged on a few apps with this?

lib/Transforms/Utils/FunctionImportUtils.cpp

89 ↗

(On Diff #83060)

Assert are compiled out in NDEBUG mode, I think it won't do anything.

You may compile out this function entirely in #ifndef NDEBUG (and its declaration) so it can't be called.

[tejohnson]

In D28169#636490, @mehdi_amini wrote:

LGTM, but have you checked that the number of imports is unchanged on a few apps with this?

Just confirmed they are the same for SPEC benchmarks gcc and xalancbmk.

lib/Transforms/Utils/FunctionImportUtils.cpp

89 ↗

(On Diff #83060)

Good point. I changed to compile out the function completely under NDEBUG.

[tejohnson]

Address last review comments.

llvm/trunk/include/llvm/IR/ModuleSummaryIndex.h

 
 /// \brief Function and variable summary information to aid decisions and
 /// implementation of importing.
 class GlobalValueSummary {
 public:
   /// \brief Sububclass discriminator (for dyn_cast<> et al.)
   enum SummaryKind : unsigned { AliasKind, FunctionKind, GlobalVarKind };
 
-  /// Group flags (Linkage, noRename, isOptSize, etc.) as a bitfield.
+  /// Group flags (Linkage, NotEligibleToImport, etc.) as a bitfield.
   struct GVFlags {
     /// \brief The linkage type of the associated global value.
     ///
     /// One use is to flag values that have local linkage types and need to
     /// have module identifier appended before placing into the combined
     /// index, to disambiguate from other values with the same name.
     /// In the future this will be used to update and optimize linkage
     /// types based on global summary-based analysis.
     unsigned Linkage : 4;
 
-    /// Indicate if the global value cannot be renamed (in a specific section,
-    /// possibly referenced from inline assembly, etc).
-    unsigned NoRename : 1;
-
-    /// Indicate if a function contains inline assembly (which is opaque),
-    /// that may reference a local value. This is used to prevent importing
-    /// of this function, since we can't promote and rename the uses of the
-    /// local in the inline assembly. Use a flag rather than bloating the
-    /// summary with references to every possible local value in the
-    /// llvm.used set.
-    unsigned HasInlineAsmMaybeReferencingInternal : 1;
-
-    /// Indicate if the function is not viable to inline.
-    unsigned IsNotViableToInline : 1;
+    /// Indicate if the global value cannot be imported (e.g. it cannot
+    /// be renamed or references something that can't be renamed).
+    unsigned NotEligibleToImport : 1;
 
     /// Convenience Constructors
-    explicit GVFlags(GlobalValue::LinkageTypes Linkage, bool NoRename,
-                     bool HasInlineAsmMaybeReferencingInternal,
-                     bool IsNotViableToInline)
-        : Linkage(Linkage), NoRename(NoRename),
-          HasInlineAsmMaybeReferencingInternal(
-              HasInlineAsmMaybeReferencingInternal),
-          IsNotViableToInline(IsNotViableToInline) {}
-
-    GVFlags(const GlobalValue &GV)
-        : Linkage(GV.getLinkage()), NoRename(GV.hasSection()),
-          HasInlineAsmMaybeReferencingInternal(false) {
-      IsNotViableToInline = false;
-      if (const auto *F = dyn_cast<Function>(&GV))
-        // Inliner doesn't handle variadic functions.
-        // FIXME: refactor this to use the same code that inliner is using.
-        IsNotViableToInline = F->isVarArg();
-    }
+    explicit GVFlags(GlobalValue::LinkageTypes Linkage,
+                     bool NotEligibleToImport)
+        : Linkage(Linkage), NotEligibleToImport(NotEligibleToImport) {}
   };
 
 private:
   /// Kind of summary for use in dyn_cast<> et al.
   SummaryKind Kind;
 
   GVFlags Flags;
 
public:
   }
 
   /// Sets the linkage to the value determined by global summary-based
   /// optimization. Will be applied in the ThinLTO backends.
   void setLinkage(GlobalValue::LinkageTypes Linkage) {
     Flags.Linkage = Linkage;
   }
 
-  bool isNotViableToInline() const { return Flags.IsNotViableToInline; }
+  /// Return true if this global value can't be imported.
+  bool notEligibleToImport() const { return Flags.NotEligibleToImport; }
 
-  /// Return true if this summary is for a GlobalValue that needs promotion
-  /// to be referenced from another module.
-  bool needsRenaming() const { return GlobalValue::isLocalLinkage(linkage()); }
-
-  /// Return true if this global value cannot be renamed (in a specific section,
-  /// possibly referenced from inline assembly, etc).
-  bool noRename() const { return Flags.NoRename; }
-
-  /// Flag that this global value cannot be renamed (in a specific section,
-  /// possibly referenced from inline assembly, etc).
-  void setNoRename() { Flags.NoRename = true; }
-
-  /// Return true if this global value possibly references another value
-  /// that can't be renamed.
-  bool hasInlineAsmMaybeReferencingInternal() const {
-    return Flags.HasInlineAsmMaybeReferencingInternal;
-  }
-
-  /// Flag that this global value possibly references another value that
-  /// can't be renamed.
-  void setHasInlineAsmMaybeReferencingInternal() {
-    Flags.HasInlineAsmMaybeReferencingInternal = true;
-  }
+  /// Flag that this global value cannot be imported.
+  void setNotEligibleToImport() { Flags.NotEligibleToImport = true; }
 
   /// Return the list of values referenced by this global value definition.
   ArrayRef<ValueInfo> refs() const { return RefEdgeList; }
 };
 
 /// \brief Alias summary information.
 class AliasSummary : public GlobalValueSummary {
   GlobalValueSummary *AliaseeSummary;

llvm/trunk/include/llvm/Transforms/Utils/FunctionImportUtils.h

class FunctionImportGlobalProcessing {
   DenseSet<const GlobalValue *> *GlobalsToImport;
 
   /// Set to true if the given ModuleSummaryIndex contains any functions
   /// from this source module, in which case we must conservatively assume
   /// that any of its functions may be imported into another module
   /// as part of a different backend compilation process.
   bool HasExportedFunctions = false;
 
+  /// Set of llvm.*used values, in order to validate that we don't try
+  /// to promote any non-renamable values.
+  SmallPtrSet<GlobalValue *, 8> Used;
+
   /// Check if we should promote the given local value to global scope.
   bool shouldPromoteLocalToGlobal(const GlobalValue *SGV);
 
+#ifndef NDEBUG
+  /// Check if the given value is a local that can't be renamed (promoted).
+  /// Only used in assertion checking, and disabled under NDEBUG since the Used
+  /// set will not be populated.
+  bool isNonRenamableLocal(const GlobalValue &GV) const;
+#endif
+
   /// Helper methods to check if we are importing from or potentially
   /// exporting from the current source module.
   bool isPerformingImport() const { return GlobalsToImport != nullptr; }
   bool isModuleExporting() const { return HasExportedFunctions; }
 
   /// If we are importing from the source module, checks if we should
   /// import SGV as a definition, otherwise import as a declaration.
   bool doImportAsDefinition(const GlobalValue *SGV);
FunctionImportGlobalProcessing(
       DenseSet<const GlobalValue *> *GlobalsToImport = nullptr)
       : M(M), ImportIndex(Index), GlobalsToImport(GlobalsToImport) {
     // If we have a ModuleSummaryIndex but no function to import,
     // then this is the primary module being compiled in a ThinLTO
     // backend compilation, and we need to see if it has functions that
     // may be exported to another backend compilation.
     if (!GlobalsToImport)
       HasExportedFunctions = ImportIndex.hasExportedFunctions(M);
+
+#ifndef NDEBUG
+    // First collect those in the llvm.used set.
+    collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);
+    // Next collect those in the llvm.compiler.used set.
+    collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ true);
+#endif
   }
 
   bool run();
 
   static bool
   doImportAsDefinition(const GlobalValue *SGV,
                        DenseSet<const GlobalValue *> *GlobalsToImport);
 };

llvm/trunk/lib/Analysis/ModuleSummaryAnalysis.cpp

if (!PSI)
     return CalleeInfo::HotnessType::Unknown;
   if (PSI->isHotCount(ProfileCount))
     return CalleeInfo::HotnessType::Hot;
   if (PSI->isColdCount(ProfileCount))
     return CalleeInfo::HotnessType::Cold;
   return CalleeInfo::HotnessType::None;
 }
 
-static void computeFunctionSummary(ModuleSummaryIndex &Index, const Module &M,
-                                   const Function &F, BlockFrequencyInfo *BFI,
-                                   ProfileSummaryInfo *PSI,
-                                   bool HasLocalsInUsed) {
+static bool isNonRenamableLocal(const GlobalValue &GV) {
+  return GV.hasSection() && GV.hasLocalLinkage();
+}
+
+static void
+computeFunctionSummary(ModuleSummaryIndex &Index, const Module &M,
+                       ProfileSummaryInfo *PSI, bool HasLocalsInUsed,
+                       SmallPtrSet<GlobalValue::GUID, 8> &CantBePromoted) {
   // Summary not currently supported for anonymous functions, they should
   // have been named.
   assert(F.hasName());
 
   unsigned NumInsts = 0;
   // Map from callee ValueId to profile count. Used to accumulate profile
   // counts for all static calls to a given callee.
   MapVector<ValueInfo, CalleeInfo> CallGraphEdges;
for (const Instruction &I : BB) {
             ICallAnalysis.getPromotionCandidatesForInstruction(
                 &I, NumVals, TotalCount, NumCandidates);
         for (auto &Candidate : CandidateProfileData)
           CallGraphEdges[Candidate.Value].updateHotness(
               getHotness(Candidate.Count, PSI));
       }
     }
 
-  GlobalValueSummary::GVFlags Flags(F);
+  bool NonRenamableLocal = isNonRenamableLocal(F);
+  bool NotEligibleForImport =
+      NonRenamableLocal || HasInlineAsmMaybeReferencingInternal ||
+      // Inliner doesn't handle variadic functions.
+      // FIXME: refactor this to use the same code that inliner is using.
+      F.isVarArg();
+  GlobalValueSummary::GVFlags Flags(F.getLinkage(), NotEligibleForImport);
   auto FuncSummary = llvm::make_unique<FunctionSummary>(
       Flags, NumInsts, RefEdges.takeVector(), CallGraphEdges.takeVector(),
       TypeTests.takeVector());
-  if (HasInlineAsmMaybeReferencingInternal)
-    FuncSummary->setHasInlineAsmMaybeReferencingInternal();
+  if (NonRenamableLocal)
+    CantBePromoted.insert(F.getGUID());
   Index.addGlobalValueSummary(F.getName(), std::move(FuncSummary));
 }
 
-static void computeVariableSummary(ModuleSummaryIndex &Index,
-                                   const GlobalVariable &V) {
+static void
+computeVariableSummary(ModuleSummaryIndex &Index, const GlobalVariable &V,
+                       SmallPtrSet<GlobalValue::GUID, 8> &CantBePromoted) {
   SetVector<ValueInfo> RefEdges;
   SmallPtrSet<const User *, 8> Visited;
   findRefEdges(&V, RefEdges, Visited);
-  GlobalValueSummary::GVFlags Flags(V);
+  bool NonRenamableLocal = isNonRenamableLocal(V);
+  GlobalValueSummary::GVFlags Flags(V.getLinkage(), NonRenamableLocal);
   auto GVarSummary =
       llvm::make_unique<GlobalVarSummary>(Flags, RefEdges.takeVector());
+  if (NonRenamableLocal)
+    CantBePromoted.insert(V.getGUID());
   Index.addGlobalValueSummary(V.getName(), std::move(GVarSummary));
 }
 
-static void computeAliasSummary(ModuleSummaryIndex &Index,
-                                const GlobalAlias &A) {
-  GlobalValueSummary::GVFlags Flags(A);
+static void
+computeAliasSummary(ModuleSummaryIndex &Index, const GlobalAlias &A,
+                    SmallPtrSet<GlobalValue::GUID, 8> &CantBePromoted) {
+  bool NonRenamableLocal = isNonRenamableLocal(A);
+  GlobalValueSummary::GVFlags Flags(A.getLinkage(), NonRenamableLocal);
   auto AS = llvm::make_unique<AliasSummary>(Flags, ArrayRef<ValueInfo>{});
   auto *Aliasee = A.getBaseObject();
   auto *AliaseeSummary = Index.getGlobalValueSummary(*Aliasee);
   assert(AliaseeSummary && "Alias expects aliasee summary to be parsed");
   AS->setAliasee(AliaseeSummary);
+  if (NonRenamableLocal)
+    CantBePromoted.insert(A.getGUID());
   Index.addGlobalValueSummary(A.getName(), std::move(AS));
 }
 
 ModuleSummaryIndex llvm::buildModuleSummaryIndex(
     const Module &M,
     std::function<BlockFrequencyInfo *(const Function &F)> GetBFICallback,
     ProfileSummaryInfo *PSI) {
   ModuleSummaryIndex Index;
 
   // Identify the local values in the llvm.used and llvm.compiler.used sets,
   // which should not be exported as they would then require renaming and
   // promotion, but we may have opaque uses e.g. in inline asm. We collect them
   // here because we use this information to mark functions containing inline
   // assembly calls as not importable.
   SmallPtrSet<GlobalValue *, 8> LocalsUsed;
   SmallPtrSet<GlobalValue *, 8> Used;
   // First collect those in the llvm.used set.
   collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);
   // Next collect those in the llvm.compiler.used set.
   collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ true);
+  SmallPtrSet<GlobalValue::GUID, 8> CantBePromoted;
   for (auto *V : Used) {
-    if (V->hasLocalLinkage())
+    if (V->hasLocalLinkage()) {
       LocalsUsed.insert(V);
+      CantBePromoted.insert(V->getGUID());
+    }
   }
 
   // Compute summaries for all functions defined in module, and save in the
   // index.
   for (auto &F : M) {
     if (F.isDeclaration())
       continue;
 
     BlockFrequencyInfo *BFI = nullptr;
     std::unique_ptr<BlockFrequencyInfo> BFIPtr;
     if (GetBFICallback)
       BFI = GetBFICallback(F);
     else if (F.getEntryCount().hasValue()) {
       LoopInfo LI{DominatorTree(const_cast<Function &>(F))};
       BranchProbabilityInfo BPI{F, LI};
       BFIPtr = llvm::make_unique<BlockFrequencyInfo>(F, BPI, LI);
       BFI = BFIPtr.get();
     }
 
-    computeFunctionSummary(Index, M, F, BFI, PSI, !LocalsUsed.empty());
+    computeFunctionSummary(Index, M, F, BFI, PSI, !LocalsUsed.empty(),
+                           CantBePromoted);
   }
 
   // Compute summaries for all variables defined in module, and save in the
   // index.
   for (const GlobalVariable &G : M.globals()) {
     if (G.isDeclaration())
       continue;
-    computeVariableSummary(Index, G);
+    computeVariableSummary(Index, G, CantBePromoted);
   }
 
   // Compute summaries for all aliases defined in module, and save in the
   // index.
   for (const GlobalAlias &A : M.aliases())
-    computeAliasSummary(Index, A);
+    computeAliasSummary(Index, A, CantBePromoted);
 
   for (auto *V : LocalsUsed) {
     auto *Summary = Index.getGlobalValueSummary(*V);
     assert(Summary && "Missing summary for global value");
-    Summary->setNoRename();
+    Summary->setNotEligibleToImport();
   }
 
   if (!M.getModuleInlineAsm().empty()) {
     // Collect the local values defined by module level asm, and set up
     // summaries for these symbols so that they can be marked as NoRename,
     // to prevent export of any use of them in regular IR that would require
     // renaming within the module level asm. Note we don't need to create a
     // summary for weak or global defs, as they don't need to be flagged as
     // NoRename, and defs in module level asm can't be imported anyway.
     // Also, any values used but not defined within module level asm should
     // be listed on the llvm.used or llvm.compiler.used global and marked as
     // referenced from there.
     ModuleSymbolTable::CollectAsmSymbols(
         Triple(M.getTargetTriple()), M.getModuleInlineAsm(),
-        [&M, &Index](StringRef Name, object::BasicSymbolRef::Flags Flags) {
+        [&M, &Index, &CantBePromoted](StringRef Name,
+                                      object::BasicSymbolRef::Flags Flags) {
           // Symbols not marked as Weak or Global are local definitions.
           if (Flags & (object::BasicSymbolRef::SF_Weak |
                        object::BasicSymbolRef::SF_Global))
             return;
           GlobalValue *GV = M.getNamedValue(Name);
           if (!GV)
             return;
           assert(GV->isDeclaration() && "Def in module asm already has definition");
-          GlobalValueSummary::GVFlags GVFlags(
-              GlobalValue::InternalLinkage,
-              /* NoRename */ true,
-              /* HasInlineAsmMaybeReferencingInternal */ false,
-              /* IsNotViableToInline */ true);
+          GlobalValueSummary::GVFlags GVFlags(GlobalValue::InternalLinkage,
+                                              /* NotEligibleToImport */ true);
+          CantBePromoted.insert(GlobalValue::getGUID(Name));
           // Create the appropriate summary type.
           if (isa<Function>(GV)) {
             std::unique_ptr<FunctionSummary> Summary =
                 llvm::make_unique<FunctionSummary>(
                     GVFlags, 0, ArrayRef<ValueInfo>{},
                     ArrayRef<FunctionSummary::EdgeTy>{},
                     ArrayRef<GlobalValue::GUID>{});
-            Summary->setNoRename();
             Index.addGlobalValueSummary(Name, std::move(Summary));
           } else {
             std::unique_ptr<GlobalVarSummary> Summary =
                 llvm::make_unique<GlobalVarSummary>(GVFlags,
                                                     ArrayRef<ValueInfo>{});
-            Summary->setNoRename();
             Index.addGlobalValueSummary(Name, std::move(Summary));
           }
         });
   }
 
+  for (auto &GlobalList : Index) {
+    assert(GlobalList.second.size() == 1 &&
+           "Expected module's index to have one summary per GUID");
+    auto &Summary = GlobalList.second[0];
+    bool AllRefsCanBeExternallyReferenced =
+        llvm::all_of(Summary->refs(), [&](const ValueInfo &VI) {
+          return !CantBePromoted.count(VI.getValue()->getGUID());
+        });
+    if (!AllRefsCanBeExternallyReferenced) {
+      Summary->setNotEligibleToImport();
+      continue;
+    }
+
+    if (auto *FuncSummary = dyn_cast<FunctionSummary>(Summary.get())) {
+      bool AllCallsCanBeExternallyReferenced = llvm::all_of(
+          FuncSummary->calls(), [&](const FunctionSummary::EdgeTy &Edge) {
+            auto GUID = Edge.first.isGUID() ? Edge.first.getGUID()
+                                            : Edge.first.getValue()->getGUID();
+            return !CantBePromoted.count(GUID);
+          });
+      if (!AllCallsCanBeExternallyReferenced)
+        Summary->setNotEligibleToImport();
+    }
+  }
+
   return Index;
 }
 
 AnalysisKey ModuleSummaryIndexAnalysis::Key;
 
 ModuleSummaryIndex
 ModuleSummaryIndexAnalysis::run(Module &M, ModuleAnalysisManager &AM) {
   ProfileSummaryInfo &PSI = AM.getResult<ProfileSummaryAnalysis>(M);

llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp

 /// Decode the flags for GlobalValue in the summary.
 static GlobalValueSummary::GVFlags getDecodedGVSummaryFlags(uint64_t RawFlags,
                                                             uint64_t Version) {
   // Summary were not emitted before LLVM 3.9, we don't need to upgrade Linkage
   // like getDecodedLinkage() above. Any future change to the linkage enum and
   // to getDecodedLinkage() will need to be taken into account here as above.
   auto Linkage = GlobalValue::LinkageTypes(RawFlags & 0xF); // 4 bits
   RawFlags = RawFlags >> 4;
-  bool NoRename = RawFlags & 0x1;
-  bool IsNotViableToInline = RawFlags & 0x2;
-  bool HasInlineAsmMaybeReferencingInternal = RawFlags & 0x4;
-  return GlobalValueSummary::GVFlags(Linkage, NoRename,
-                                     HasInlineAsmMaybeReferencingInternal,
-                                     IsNotViableToInline);
+  bool NotEligibleToImport = (RawFlags & 0x1) || Version < 3;
+  return GlobalValueSummary::GVFlags(Linkage, NotEligibleToImport);
 }
 
 static GlobalValue::VisibilityTypes getDecodedVisibility(unsigned Val) {
   switch (Val) {
   default: // Map unknown visibilities to default.
   case 0: return GlobalValue::DefaultVisibility;
   case 1: return GlobalValue::HiddenVisibility;
   case 2: return GlobalValue::ProtectedVisibility;
// Parse version
     BitstreamEntry Entry = Stream.advanceSkippingSubblocks();
     if (Entry.Kind != BitstreamEntry::Record)
       return error("Invalid Summary Block: record for version expected");
     if (Stream.readRecord(Entry.ID, Record) != bitc::FS_VERSION)
       return error("Invalid Summary Block: version expected");
   }
   const uint64_t Version = Record[0];
   const bool IsOldProfileFormat = Version == 1;
-  if (!IsOldProfileFormat && Version != 2)
+  if (Version < 1 || Version > 3)
     return error("Invalid summary version " + Twine(Version) +
-                 ", 1 or 2 expected");
+                 ", 1, 2 or 3 expected");
   Record.clear();
 
   // Keep around the last seen summary to be used when we see an optional
   // "OriginalName" attachement.
   GlobalValueSummary *LastSeenSummary = nullptr;
   bool Combined = false;
   std::vector<GlobalValue::GUID> PendingTypeTests;
 

llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp

 static unsigned getEncodedLinkage(const GlobalValue &GV) {
   return getEncodedLinkage(GV.getLinkage());
 }
 
 // Decode the flags for GlobalValue in the summary
 static uint64_t getEncodedGVSummaryFlags(GlobalValueSummary::GVFlags Flags) {
   uint64_t RawFlags = 0;
 
-  RawFlags |= Flags.NoRename; // bool
-  RawFlags |= (Flags.IsNotViableToInline << 1);
-  RawFlags |= (Flags.HasInlineAsmMaybeReferencingInternal << 2);
+  RawFlags |= Flags.NotEligibleToImport; // bool
   // Linkage don't need to be remapped at that time for the summary. Any future
   // change to the getEncodedLinkage() function will need to be taken into
   // account here as well.
   RawFlags = (RawFlags << 4) | Flags.Linkage; // 4 bits
 
   return RawFlags;
 }
 
void ModuleBitcodeWriter::writeModuleLevelReferences(
   Stream.EmitRecord(bitc::FS_PERMODULE_GLOBALVAR_INIT_REFS, NameVals,
                     FSModRefsAbbrev);
   NameVals.clear();
 }
 
 // Current version for the summary.
 // This is bumped whenever we introduce changes in the way some record are
 // interpreted, like flags for instance.
-static const uint64_t INDEX_VERSION = 2;
+static const uint64_t INDEX_VERSION = 3;
 
 /// Emit the per-module summary section alongside the rest of
 /// the module's bitcode.
 void ModuleBitcodeWriter::writePerModuleGlobalValueSummary() {
   Stream.EnterSubblock(bitc::GLOBALVAL_SUMMARY_BLOCK_ID, 4);
 
   Stream.EmitRecord(bitc::FS_VERSION, ArrayRef<uint64_t>{INDEX_VERSION});
 

llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp

if (!Result) {
     report_fatal_error("Abort");
   }
 
   return Result;
 }
 
 namespace {
 
-// Return true if the Summary describes a GlobalValue that can be externally
-// referenced, i.e. it does not need renaming (linkage is not local) or renaming
-// is possible (does not have a section for instance).
-static bool canBeExternallyReferenced(const GlobalValueSummary &Summary) {
-  if (!Summary.needsRenaming())
-    return true;
-
-  if (Summary.noRename())
-    // Can't externally reference a global that needs renaming if has a section
-    // or is referenced from inline assembly, for example.
-    return false;
-
-  return true;
-}
-
-// Return true if \p GUID describes a GlobalValue that can be externally
-// referenced, i.e. it does not need renaming (linkage is not local) or
-// renaming is possible (does not have a section for instance).
-static bool canBeExternallyReferenced(const ModuleSummaryIndex &Index,
-                                      GlobalValue::GUID GUID) {
-  auto Summaries = Index.findGlobalValueSummaryList(GUID);
-  if (Summaries == Index.end())
-    return true;
-  if (Summaries->second.size() != 1)
-    // If there are multiple globals with this GUID, then we know it is
-    // not a local symbol, and it is necessarily externally referenced.
-    return true;
-
-  // We don't need to check for the module path, because if it can't be
-  // externally referenced and we call it, it is necessarilly in the same
-  // module
-  return canBeExternallyReferenced(**Summaries->second.begin());
-}
-
-// Return true if the global described by \p Summary can be imported in another
-// module.
-static bool eligibleForImport(const ModuleSummaryIndex &Index,
-                              const GlobalValueSummary &Summary) {
-  if (!canBeExternallyReferenced(Summary))
-    // Can't import a global that needs renaming if has a section for instance.
-    // FIXME: we may be able to import it by copying it without promotion.
-    return false;
-
-  // Don't import functions that are not viable to inline.
-  if (Summary.isNotViableToInline())
-    return false;
-
-  // Check references (and potential calls) in the same module. If the current
-  // value references a global that can't be externally referenced it is not
-  // eligible for import. First check the flag set when we have possible
-  // opaque references (e.g. inline asm calls), then check the call and
-  // reference sets.
-  if (Summary.hasInlineAsmMaybeReferencingInternal())
-    return false;
-  bool AllRefsCanBeExternallyReferenced =
-      llvm::all_of(Summary.refs(), [&](const ValueInfo &VI) {
-        return canBeExternallyReferenced(Index, VI.getGUID());
-      });
-  if (!AllRefsCanBeExternallyReferenced)
-    return false;
-
-  if (auto *FuncSummary = dyn_cast<FunctionSummary>(&Summary)) {
-    bool AllCallsCanBeExternallyReferenced = llvm::all_of(
-        FuncSummary->calls(), [&](const FunctionSummary::EdgeTy &Edge) {
-          return canBeExternallyReferenced(Index, Edge.first.getGUID());
-        });
-    if (!AllCallsCanBeExternallyReferenced)
-      return false;
-  }
-  return true;
-}
-
 /// Given a list of possible callee implementation for a call site, select one
 /// that fits the \p Threshold.
 ///
 /// FIXME: select "best" instead of first that fits. But what is "best"?
 /// - The smallest: more likely to be inlined.
 /// - The one with the least outgoing edges (already well optimized).
 /// - One from a module already being imported from in order to reduce the
 ///   number of source modules parsed/linked.
auto It = llvm::find_if(
             return false;
         }
 
         auto *Summary = cast<FunctionSummary>(GVSummary);
 
         if (Summary->instCount() > Threshold)
           return false;
 
-        if (!eligibleForImport(Index, *Summary))
+        if (Summary->notEligibleToImport())
           return false;
 
         return true;
       });
   if (It == CalleeSummaryList.end())
     return nullptr;
 
   return cast<GlobalValueSummary>(It->get());

llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp

 bool FunctionImportGlobalProcessing::shouldPromoteLocalToGlobal(
     const GlobalValue *SGV) {
   assert(SGV->hasLocalLinkage());
   // Both the imported references and the original local variable must
   // be promoted.
   if (!isPerformingImport() && !isModuleExporting())
     return false;
 
-  // If we are exporting, we need to see whether this value is marked
-  // as NoRename in the summary. If we are importing, we may not have
-  // a summary in the distributed backend case (only summaries for values
-  // importes as defs, not references, are included in the index passed
-  // to the distributed backends).
   if (isPerformingImport()) {
+    assert(!GlobalsToImport->count(SGV) ||
+           !isNonRenamableLocal(*SGV) &&
+               "Attempting to promote non-renamable local");
     // We don't know for sure yet if we are importing this value (as either
     // a reference or a def), since we are simply walking all values in the
     // module. But by necessity if we end up importing it and it is local,
     // it must be promoted, so unconditionally promote all values in the
     // importing module.
     return true;
   }
 
   // When exporting, consult the index.
   auto Summaries = ImportIndex.findGlobalValueSummaryList(SGV->getGUID());
   assert(Summaries != ImportIndex.end() &&
          "Missing summary for global value when exporting");
   assert(Summaries->second.size() == 1 && "Local has more than one summary");
   auto Linkage = Summaries->second.front()->linkage();
   if (!GlobalValue::isLocalLinkage(Linkage)) {
-    assert(!Summaries->second.front()->noRename());
+    assert(!isNonRenamableLocal(*SGV) &&
+           "Attempting to promote non-renamable local");
     return true;
   }
 
   return false;
 }
 
+#ifndef NDEBUG
+bool FunctionImportGlobalProcessing::isNonRenamableLocal(
+    const GlobalValue &GV) const {
+  if (!GV.hasLocalLinkage())
+    return false;
+  // This needs to stay in sync with the logic in buildModuleSummaryIndex.
+  if (GV.hasSection())
+    return true;
+  if (Used.count(const_cast<GlobalValue *>(&GV)))
+    return true;
+  return false;
+}
+#endif
+
 std::string FunctionImportGlobalProcessing::getName(const GlobalValue *SGV,
                                                     bool DoPromote) {
   // For locals that must be promoted to global scope, ensure that
   // the promoted name uniquely identifies the copy in the original module,
   // using the ID assigned during combined index creation. When importing,
   // we rename all locals (not just those that are promoted) in order to
   // avoid naming conflicts between locals imported from different modules.
   if (SGV->hasLocalLinkage() && (DoPromote || isPerformingImport()))

llvm/trunk/test/Bitcode/summary_version.ll

 ; Check summary versioning
 ; RUN: opt  -module-summary  %s -o - | llvm-bcanalyzer -dump | FileCheck %s
 
 ; CHECK: <GLOBALVAL_SUMMARY_BLOCK
-; CHECK: <VERSION op0=2/>
+; CHECK: <VERSION op0=3/>
 
 
 
 ; Need a function for the summary to be populated.
 define void @foo() {
     ret void
 }

llvm/trunk/test/Bitcode/thinlto-function-summary.ll

 ; RUN: opt -name-anon-globals -module-summary < %s | llvm-bcanalyzer -dump | FileCheck %s -check-prefix=BC
 ; RUN: opt -passes=name-anon-globals -module-summary < %s | llvm-bcanalyzer -dump | FileCheck %s -check-prefix=BC
 ; Check for summary block/records.
 
 ; Check the value ids in the summary entries against the
 ; same in the ValueSumbolTable, to ensure the ordering is stable.
 ; Also check the linkage field on the summary entries.
 ; BC: <GLOBALVAL_SUMMARY_BLOCK
 ; BC-NEXT: <VERSION
 ; BC-NEXT: <PERMODULE {{.*}} op0=1 op1=0
 ; BC-NEXT: <PERMODULE {{.*}} op0=2 op1=0
 ; BC-NEXT: <PERMODULE {{.*}} op0=3 op1=7
-; BC-NEXT: <PERMODULE {{.*}} op0=4 op1=32
+; BC-NEXT: <PERMODULE {{.*}} op0=4 op1=16
 ; BC-NEXT: <ALIAS {{.*}} op0=5 op1=0 op2=3
 ; BC-NEXT: </GLOBALVAL_SUMMARY_BLOCK
 ; BC-NEXT: <VALUE_SYMTAB
 ; BC-NEXT: <FNENTRY {{.*}} op0=4 {{.*}}> record string = 'variadic'
 ; BC-NEXT: <FNENTRY {{.*}} op0=1 {{.*}}> record string = 'foo'
 ; BC-NEXT: <FNENTRY {{.*}} op0=2 {{.*}}> record string = 'bar'
 ; BC-NEXT: <FNENTRY {{.*}} op0=5 {{.*}}> record string = 'f'
 ; BC-NEXT: <ENTRY {{.*}} record string = 'h'

llvm/trunk/test/Bitcode/thinlto-summary-section.ll

 ; Check the linkage types in both the per-module and combined summaries.
 ; RUN: opt -module-summary %s -o %t.o
 ; RUN: llvm-bcanalyzer -dump %t.o | FileCheck %s
 ; RUN: llvm-lto -thinlto -o %t2 %t.o
 ; RUN: llvm-bcanalyzer -dump %t2.thinlto.bc | FileCheck %s --check-prefix=COMBINED
 
-; CHECK: <PERMODULE {{.*}} op1=16
-; COMBINED-DAG: <COMBINED {{.*}} op2=16
-define void @functionWithSection() section "some_section" {
+; Flags should be 0x17 (23) for local linkage (0x3) and not being importable
+; (0x10) due to local linkage plus having a section.
+; CHECK: <PERMODULE {{.*}} op1=23
+; COMBINED-DAG: <COMBINED {{.*}} op2=23
+define internal void @functionWithSection() section "some_section" {
     ret void
 }