[LTO] Drop non-prevailing definitions only if linkage is not local or appending
ClosedPublic

Authored by pirama on Mon, Nov 5, 2:52 PM.

Details

Summary

This fixes PR 37422

In ELF, non-weak symbols can also be non-prevailing. In this particular
PR, the __llvm_profile_* symbols are non-prevailing but weren't getting
dropped - causing multiply-defined errors with lld.

Also add a test, strong_non_prevailing.ll, to ensure that multiple
copies of a strong symbol are dropped.

To fix the test regressions exposed by this fix,

  • do not mark prevailing copies for symbols with 'appending' linkage.

There's no one prevailing copy for such symbols.

  • fix the prevailing version in dead-strip-fulllto.ll
  • explicitly pass exported symbols to llvm-lto in fumcimport.ll and

funcimport_var.ll

Diff Detail

Repository
rL LLVM
pirama created this revision.Mon, Nov 5, 2:52 PM
pirama updated this revision to Diff 172674.Mon, Nov 5, 4:07 PM

Fix commit message.

tejohnson added inline comments.Tue, Nov 6, 6:39 AM
lib/LTO/LTO.cpp
285 ↗(On Diff #172674)

The thinLTOResolveWeakForLinker* routines need a new name with this change. Maybe thinLTOResolvePrevailing*?

294 ↗(On Diff #172674)

You mentioned in the PR that you get test failures if you guard against appending linkage symbols here. I'd like to understand why. With the old code we would not have resolved appending linkage symbols either.

pirama added inline comments.Tue, Nov 6, 9:23 AM
lib/LTO/LTO.cpp
294 ↗(On Diff #172674)

Adding the guard here was fine. But, per your suggstion, I replaced the guard with the change to ThinLTOCodeGenerator.cpp:computePrevailingCopies that skips adding prevailing symbols for appending-linkage symbols.

I'll leave a comment at the lines that tripped the tests.

353 ↗(On Diff #172674)

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.

lib/Transforms/IPO/FunctionImport.cpp
915 ↗(On Diff #172674)

This is the check I wanted to eliminate.

pirama updated this revision to Diff 172852.Tue, Nov 6, 2:10 PM

Rename thinLTOResolveWeakForLinker* to thinLTOResolvePrevailing*

pirama marked 4 inline comments as done.Tue, Nov 6, 2:13 PM
pirama added inline comments.
lib/LTO/LTO.cpp
353 ↗(On Diff #172674)

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.

smeenai added a subscriber: smeenai.Tue, Nov 6, 6:08 PM
tejohnson added inline comments.Tue, Nov 6, 7:00 PM
lib/LTO/LTO.cpp
353 ↗(On Diff #172674)

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?

pirama added inline comments.Tue, Nov 6, 10:14 PM
lib/LTO/LTO.cpp
353 ↗(On Diff #172674)

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.

lib/Transforms/IPO/FunctionImport.cpp
898 ↗(On Diff #172852)

The InternalLinkage set in thinLTOInternalizeAndPromoteGUID is read here.

942 ↗(On Diff #172852)

If I don't have the guard for AppendingLinkage earlier in this function, control flow reaches this line and the variable's linkage gets set to InternalLinkage.

tejohnson added inline comments.Wed, Nov 7, 7:04 AM
lib/LTO/LTO.cpp
353 ↗(On Diff #172674)

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?

pirama added inline comments.Wed, Nov 7, 11:26 AM
lib/LTO/LTO.cpp
353 ↗(On Diff #172674)

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.

tejohnson added inline comments.Wed, Nov 7, 1:19 PM
lib/LTO/LTO.cpp
318 ↗(On Diff #172852)

Please update the comment.

353 ↗(On Diff #172674)

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
lib/Transforms/IPO/FunctionImport.cpp
891 ↗(On Diff #172852)

Please rename consistently with the index-based routines in LTO.cpp (and adjust comment).

pirama updated this revision to Diff 173123.Thu, Nov 8, 12:06 AM

Update based on review comments. Hopefully I did the WeakForLinker->Prevailing
name changes in all the right places.

tejohnson accepted this revision.Thu, Nov 8, 6:25 AM

LGTM, just a few comments that need to be updated before submit. Thanks!

lib/LTO/LTO.cpp
318 ↗(On Diff #172852)

This comment still needs a fix.

lib/LTO/ThinLTOCodeGenerator.cpp
503 ↗(On Diff #173123)

Update comment

678 ↗(On Diff #173123)

Comment needs update.

This revision is now accepted and ready to land.Thu, Nov 8, 6:25 AM
pirama updated this revision to Diff 173201.Thu, Nov 8, 11:05 AM

Fix a few more comments.

Thanks for the review Teresa. I'm running the tests again after a rebase and will commit this after the tests pass.

From the summary,

"In ELF, symbols non-weak symbols"

The first "symbols" seems extraneous?

From the summary,

"In ELF, symbols non-weak symbols"

The first "symbols" seems extraneous?

I fixed it in the commit message, but Phabricator seems to showing the commit message from the first patch. I'll manually update the summary.

pirama retitled this revision from [LTO] Drop non-prevailing definitions for non-local linkage types to [LTO] Drop non-prevailing definitions only if linkage is not local or appending.Thu, Nov 8, 11:10 AM
pirama edited the summary of this revision. (Show Details)

From the summary,

"In ELF, symbols non-weak symbols"

The first "symbols" seems extraneous?

I fixed it in the commit message, but Phabricator seems to showing the commit message from the first patch. I'll manually update the summary.

Yeah, Phabricator doesn't update the title or summary from the commit (after the initial diff creation) unless you pass --verbatim to --arc-diff. Doing so also restores the reviewers, subscribers, etc. from the original commit message, so it can end up dropping people who added themselves later.

This revision was automatically updated to reflect the committed changes.