[PATCH] D54125: [LTO] Drop non-prevailing definitions for non-local linkage types
Teresa Johnson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 7 13:19:38 PST 2018
tejohnson added inline comments.
================
Comment at: lib/LTO/LTO.cpp:353
} else if (EnableLTOInternalization &&
!GlobalValue::isLocalLinkage(S->linkage()))
S->setLinkage(GlobalValue::InternalLinkage);
----------------
pirama wrote:
> tejohnson wrote:
> > pirama wrote:
> > > tejohnson wrote:
> > > > pirama wrote:
> > > > > pirama wrote:
> > > > > > If I skip appending-linkage summaries here, so I can instead get rid of the AppendingLinkage check in FunctionImport.cpp:thinLTOResolveWeakForLinkerModule (line 914), I get some test failures.
> > > > > >
> > > > > > My reasoning was that this line sets linkage to Internal for summaries of appending-linkage symbols, which then gets propagated to the actual symbol in FunctionImport.cpp:thinLTOResolveWeakForLinkerModule. Not setting the linkage here will obviate the need for that check.
> > > > > >
> > > > > > I just realized this line is slightly different from my prior test - I must not have synced recently. I'll recreate this scenario and report on the tests that failed.
> > > > > On top of the current patch, if I apply the diff below:
> > > > > ```
> > > > > diff --git a/lib/LTO/LTO.cpp b/lib/LTO/LTO.cpp
> > > > > index 2c3c7359dc0..347f1a56a25 100644
> > > > > --- a/lib/LTO/LTO.cpp
> > > > > +++ b/lib/LTO/LTO.cpp
> > > > > @@ -350,7 +350,8 @@ static void thinLTOInternalizeAndPromoteGUID(
> > > > > if (GlobalValue::isLocalLinkage(S->linkage()))
> > > > > S->setLinkage(GlobalValue::ExternalLinkage);
> > > > > } else if (EnableLTOInternalization &&
> > > > > - !GlobalValue::isLocalLinkage(S->linkage()))
> > > > > + !GlobalValue::isLocalLinkage(S->linkage()) &&
> > > > > + !GlobalValue::isAppendingLinkage(S->linkage()))
> > > > > S->setLinkage(GlobalValue::InternalLinkage);
> > > > > }
> > > > > }
> > > > > diff --git a/lib/Transforms/IPO/FunctionImport.cpp b/lib/Transforms/IPO/FunctionImport.cpp
> > > > > index 5816633db97..f7c380c8a09 100644
> > > > > --- a/lib/Transforms/IPO/FunctionImport.cpp
> > > > > +++ b/lib/Transforms/IPO/FunctionImport.cpp
> > > > > @@ -912,7 +912,6 @@ void llvm::thinLTOResolveWeakForLinkerModule(
> > > > > }
> > > > >
> > > > > if (GlobalValue::isLocalLinkage(GV.getLinkage()) ||
> > > > > - GlobalValue::isAppendingLinkage(GV.getLinkage()) ||
> > > > > // In case it was dead and already converted to declaration.
> > > > > GV.isDeclaration())
> > > > > return;
> > > > > ```
> > > > >
> > > > > the following tests fail:
> > > > >
> > > > > ```
> > > > > LLVM :: ThinLTO/X86/module_asm2.ll
> > > > > LLVM :: tools/gold/X86/comdat.ll
> > > > > LLVM :: tools/gold/X86/visibility.ll
> > > > > cfi-devirt-lld-thinlto-x86_64 :: cross-dso/stats.cpp
> > > > > cfi-devirt-thinlto-i386 :: cross-dso/stats.cpp
> > > > > cfi-devirt-thinlto-newpm-i386 :: cross-dso/stats.cpp
> > > > > cfi-devirt-thinlto-newpm-x86_64 :: cross-dso/stats.cpp
> > > > > cfi-devirt-thinlto-x86_64 :: cross-dso/stats.cpp
> > > > > cfi-standalone-lld-thinlto-x86_64 :: cross-dso/stats.cpp
> > > > > cfi-standalone-lld-thinlto-x86_64 :: stats.cpp
> > > > > cfi-standalone-thinlto-i386 :: cross-dso/stats.cpp
> > > > > cfi-standalone-thinlto-i386 :: stats.cpp
> > > > > cfi-standalone-thinlto-newpm-i386 :: cross-dso/stats.cpp
> > > > > cfi-standalone-thinlto-newpm-i386 :: stats.cpp
> > > > > cfi-standalone-thinlto-newpm-x86_64 :: cross-dso/stats.cpp
> > > > > cfi-standalone-thinlto-newpm-x86_64 :: stats.cpp
> > > > > cfi-standalone-thinlto-x86_64 :: cross-dso/stats.cpp
> > > > > cfi-standalone-thinlto-x86_64 :: stats.cpp
> > > > > ```
> > > > >
> > > > > If the diff above is preferable over the current patch, I'll look further into why the tests break.
> > > > I'd like to understand what is causing the appending linkage variable to be internalized if you don't guard against appending linkage in the thinLTOResolveWeakForLinkerModule. At HEAD, we skip appending linkage in the thinLTOResolveWeakForLinker.* routines since it isn't weak linkage, and don't need to guard against them in the thinLTOInternalizeAndPromote* either.
> > > >
> > > > With your change to computePrevailingCopies, any appending linkage should be prevailing, since they won't be in the map, and thinLTOResolveWeakForLinkerGUID should theoretically not do anything since they aren't LinkOnceLinkage - which should be the same net result as the code at HEAD for those variables. Why is it now being eventually marked internal in the index whereas before it apparently wasn't?
> > > The reason is that in thinLTOInternalizeAndPromoteGUID, in the lines around the current comment, the Summary is marked as having InternalLinkage. Subsequently, in thinLTOResolveWeakForLinkerGUID , the Summary's linkage is read from the DefinedGlobals parameter. In that function, NewLinkage is InternalLinkage and gets set for the symbol itself.
> > I see - at HEAD the marking of the InternalLinkage in the summary has no effect on these appending variables since thinLTOInternalizeModule invokes llvm::internalizeModule which in turn special cases these @llvm.* special variables.
> >
> > My preference then would be to make a change in the internalization handling, rather than work around this with a chance in the weak resolution (now prevailing) handling. So let's try to understand what is causing the failures you noted with the above patch applied. I looked at the first 3 test cases:
> >
> > LLVM :: ThinLTO/X86/module_asm2.ll
> >
> > This one has @llvm*.used appending variables. What is the failure mode when you apply your patch?
> >
> > LLVM :: tools/gold/X86/comdat.ll
> > LLVM :: tools/gold/X86/visibility.ll
> >
> > The above two don't have any appending variables, so I'm not sure why they are affected. These 2 tests fail without a recent version of gold with a bugfix - are they passing for you without the above patch applied?
> module_asm2.ll fails the check on %t.0.1. Symbol `b`, the one added to `llvm.used` no longer gets exported.
>
> ```
> ../llvm/test/ThinLTO/X86/module_asm2.ll:37:12: error: NM0-DAG: expected string not found in input
> ; NM0-DAG: d b
> ^
> <stdin>:1:2: note: scanning from here
> U b
> ^
> ```
>
> The tools/gold/* failures are due to the unpatched ld.gold. I knew that but forgot to remove it when I made the comment - sorry.
Thanks for the info. I decided to apply the patch and take a look rather than keep asking for more info. =) It turns out this time it is the new LTO API (via llvm-lto2) that is doing the wrong thing, because as I see now, the linker doesn't know anything about the @llvm.*used appending linkage variables and doesn't resolve them. We could do something like what I had suggested for the old LTO API (i.e. your change to ThinLTOCodeGenerator.cpp), but it would require a little bit more work there because of the way the map is set up.
As a result, I am now thinking it is better just to special case the appending linkage in the thinLTO*InGUID routines (just as we are for local linkage) - sorry for the churn. Along with a comment in both places that the guard is necessary because the linker doesn't resolve locals and appending linkage values.
With that change the appending linkage guard in thinLTOResolveWeakForLinkerModule can be removed, along with the change I had suggested to ThinLTOCodeGenerator.cpp.
Here's my updated patch with the source changes (tests are clean):
diff --git a/include/llvm/LTO/LTO.h b/include/llvm/LTO/LTO.h
index 7d6beab6b44..e9db6276cee 100644
--- a/include/llvm/LTO/LTO.h
+++ b/include/llvm/LTO/LTO.h
@@ -46,7 +46,7 @@ class raw_pwrite_stream;
///
/// This is done for correctness (if value exported, ensure we always
/// emit a copy), and compile-time optimization (allow drop of duplicates).
-void thinLTOResolveWeakForLinkerInIndex(
+void thinLTOResolvePrevailingInIndex(
ModuleSummaryIndex &Index,
function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
isPrevailing,
diff --git a/lib/LTO/LTO.cpp b/lib/LTO/LTO.cpp
index 2726b6785ed..b60d7a1f813 100644
--- a/lib/LTO/LTO.cpp
+++ b/lib/LTO/LTO.cpp
@@ -282,7 +282,7 @@ static void computeCacheKey(
Key = toHex(Hasher.result());
}
-static void thinLTOResolveWeakForLinkerGUID(
+static void thinLTOResolvePrevailingGUID(
GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
DenseSet<GlobalValueSummary *> &GlobalInvolvedWithAlias,
function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
@@ -291,7 +291,10 @@ static void thinLTOResolveWeakForLinkerGUID(
recordNewLinkage) {
for (auto &S : GVSummaryList) {
GlobalValue::LinkageTypes OriginalLinkage = S->linkage();
- if (!GlobalValue::isWeakForLinker(OriginalLinkage))
+ // Ignore local and appending linkage values since the linker
+ // doesn't resolve them.
+ if (GlobalValue::isLocalLinkage(OriginalLinkage) ||
+ GlobalValue::isAppendingLinkage(S->linkage()))
continue;
// We need to emit only one of these. The prevailing module will keep it,
// but turned into a weak, while the others will drop it when possible.
@@ -321,7 +324,7 @@ static void thinLTOResolveWeakForLinkerGUID(
// current module. However there is a chance that another module is still
// referencing them because of the import. We make sure we always emit at least
// one copy.
-void llvm::thinLTOResolveWeakForLinkerInIndex(
+void llvm::thinLTOResolvePrevailingInIndex(
ModuleSummaryIndex &Index,
function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
isPrevailing,
@@ -337,7 +340,7 @@ void llvm::thinLTOResolveWeakForLinkerInIndex(
GlobalInvolvedWithAlias.insert(&AS->getAliasee());
for (auto &I : Index)
- thinLTOResolveWeakForLinkerGUID(I.second.SummaryList, I.first,
+ thinLTOResolvePrevailingGUID(I.second.SummaryList, I.first,
GlobalInvolvedWithAlias, isPrevailing,
recordNewLinkage);
}
@@ -350,7 +353,10 @@ static void thinLTOInternalizeAndPromoteGUID(
if (GlobalValue::isLocalLinkage(S->linkage()))
S->setLinkage(GlobalValue::ExternalLinkage);
} else if (EnableLTOInternalization &&
- !GlobalValue::isLocalLinkage(S->linkage()))
+ // Ignore local and appending linkage values since the linker
+ // doesn't resolve them.
+ !GlobalValue::isLocalLinkage(S->linkage()) &&
+ !GlobalValue::isAppendingLinkage(S->linkage()))
S->setLinkage(GlobalValue::InternalLinkage);
}
}
@@ -1205,7 +1211,7 @@ Error LTO::runThinLTO(AddStreamFn AddStream, NativeObjectCache Cache) {
GlobalValue::LinkageTypes NewLinkage) {
ResolvedODR[ModuleIdentifier][GUID] = NewLinkage;
};
- thinLTOResolveWeakForLinkerInIndex(ThinLTO.CombinedIndex, isPrevailing,
+ thinLTOResolvePrevailingInIndex(ThinLTO.CombinedIndex, isPrevailing,
recordNewLinkage);
std::unique_ptr<ThinBackendProc> BackendProc =
diff --git a/lib/LTO/ThinLTOCodeGenerator.cpp b/lib/LTO/ThinLTOCodeGenerator.cpp
index 9500b2ded70..e8743a25e6c 100644
--- a/lib/LTO/ThinLTOCodeGenerator.cpp
+++ b/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -527,7 +527,7 @@ static void resolveWeakForLinkerInIndex(
ResolvedODR[ModuleIdentifier][GUID] = NewLinkage;
};
- thinLTOResolveWeakForLinkerInIndex(Index, isPrevailing, recordNewLinkage);
+ thinLTOResolvePrevailingInIndex(Index, isPrevailing, recordNewLinkage);
}
// Initialize the TargetMachine builder for a given Triple
diff --git a/lib/Transforms/IPO/FunctionImport.cpp b/lib/Transforms/IPO/FunctionImport.cpp
index 31531beea5e..b4b16713fe0 100644
--- a/lib/Transforms/IPO/FunctionImport.cpp
+++ b/lib/Transforms/IPO/FunctionImport.cpp
@@ -921,7 +921,9 @@ void llvm::thinLTOResolveWeakForLinkerModule(
return;
}
- if (!GlobalValue::isWeakForLinker(GV.getLinkage()))
+ if (GlobalValue::isLocalLinkage(GV.getLinkage()) ||
+ // In case it was dead and already converted to declaration.
+ GV.isDeclaration())
return;
// Check for a non-prevailing def that has interposable linkage
// (e.g. non-odr weak or linkonce). In that case we can't simply
================
Comment at: lib/LTO/LTO.cpp:318
// Resolve Weak and LinkOnce values in the \p Index.
//
----------------
Please update the comment.
================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:891
/// Fixup WeakForLinker linkages in \p TheModule based on summary analysis.
void llvm::thinLTOResolveWeakForLinkerModule(
Module &TheModule, const GVSummaryMapTy &DefinedGlobals) {
----------------
Please rename consistently with the index-based routines in LTO.cpp (and adjust comment).
Repository:
rL LLVM
https://reviews.llvm.org/D54125
More information about the llvm-commits
mailing list