[ThinLTO] Simplify checking for single external copy (NFCI) #164861
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Replace a loop over all summary copies with a simple check for a single externally available copy of a symbol. The usage of this result has changed since it was added and we now only need to know if there is a single one.
Member
|
@llvm/pr-subscribers-lto Author: Teresa Johnson (teresajohnson) ChangesReplace a loop over all summary copies with a simple check for a single Full diff: https://github.com/llvm/llvm-project/pull/164861.diff 1 Files Affected:
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index 9d0fa116c85bf..7e4436a47f70d 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -471,16 +471,14 @@ static void thinLTOInternalizeAndPromoteGUID(
ValueInfo VI, function_ref<bool(StringRef, ValueInfo)> isExported,
function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
isPrevailing) {
- auto ExternallyVisibleCopies =
- llvm::count_if(VI.getSummaryList(),
- [](const std::unique_ptr<GlobalValueSummary> &Summary) {
- return !GlobalValue::isLocalLinkage(Summary->linkage());
- });
-
// Before performing index-based internalization and promotion for this GUID,
// the local flag should be consistent with the summary list linkage types.
VI.verifyLocal();
+ bool SingleExternallyVisibleCopy =
+ VI.getSummaryList().size() == 1 &&
+ !GlobalValue::isLocalLinkage(VI.getSummaryList().front()->linkage());
+
for (auto &S : VI.getSummaryList()) {
// First see if we need to promote an internal value because it is not
// exported.
@@ -543,7 +541,9 @@ static void thinLTOInternalizeAndPromoteGUID(
GlobalValue::isExternalWeakLinkage(S->linkage()))
continue;
- if (isPrevailing(VI.getGUID(), S.get()) && ExternallyVisibleCopies == 1)
+ // We may have a single summary copy that is externally visible but not
+ // prevailing if the prevailing copy is in a native object.
+ if (SingleExternallyVisibleCopy && isPrevailing(VI.getGUID(), S.get()))
S->setLinkage(GlobalValue::InternalLinkage);
}
}
|
mingmingl-llvm
approved these changes
Oct 23, 2025
Contributor
There was a problem hiding this comment.
LGTM since compiler won't internalize more by reading the diff itself.
If I understand correctly, the surrounding code and comment explains why ExternallyVisibleCopies before and SingleExternallyVisibleCopy are equivalent, and this change won't cause compiler to internalize less, most notably
llvm-project/llvm/lib/LTO/LTO.cpp
Lines 512 to 518 in ad75b3b
llvm-project/llvm/lib/LTO/LTO.cpp
Lines 538 to 544 in ad75b3b
llvm/lib/LTO/LTO.cpp
Outdated
| // the local flag should be consistent with the summary list linkage types. | ||
| VI.verifyLocal(); | ||
|
|
||
| bool SingleExternallyVisibleCopy = |
Contributor
There was a problem hiding this comment.
Optional suggestion: make this const bool so it's clearer it doesn't get modified later.
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/129/builds/32102 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/22206 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/4/builds/9973 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace a loop over all summary copies with a simple check for a single
externally available copy of a symbol. The usage of this result has
changed since it was added and we now only need to know if there is a
single one.