[ctxprof] Support for "move" semantics for the contextual root #134192
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
This was referenced Apr 3, 2025
mtrofin
force-pushed
the
users/mtrofin/04-02-_ctxprof_support_for_move_semantics_for_the_contextual_root
branch
from
aab3649 to
f9b3bfa
Compare
llvmbot
added
LTO
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-lto Author: Mircea Trofin (mtrofin) ChangesThis PR finishes what PR #133992 started. Full diff: https://github.com/llvm/llvm-project/pull/134192.diff 4 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Utils/FunctionImportUtils.h b/llvm/include/llvm/Transforms/Utils/FunctionImportUtils.h
index 6d83b615d5f13..28ba20bc18cf9 100644
--- a/llvm/include/llvm/Transforms/Utils/FunctionImportUtils.h
+++ b/llvm/include/llvm/Transforms/Utils/FunctionImportUtils.h
@@ -97,29 +97,14 @@ class FunctionImportGlobalProcessing {
/// linkage for a required promotion of a local to global scope.
GlobalValue::LinkageTypes getLinkage(const GlobalValue *SGV, bool DoPromote);
+ /// The symbols with these names are moved to a different module and should be
+ /// promoted to external linkage where they are defined.
+ DenseSet<GlobalValue::GUID> SymbolsToMove;
+
public:
FunctionImportGlobalProcessing(Module &M, const ModuleSummaryIndex &Index,
SetVector<GlobalValue *> *GlobalsToImport,
- bool ClearDSOLocalOnDeclarations)
- : M(M), ImportIndex(Index), GlobalsToImport(GlobalsToImport),
- ClearDSOLocalOnDeclarations(ClearDSOLocalOnDeclarations) {
- // 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
- SmallVector<GlobalValue *, 4> Vec;
- // First collect those in the llvm.used set.
- collectUsedGlobalVariables(M, Vec, /*CompilerUsed=*/false);
- // Next collect those in the llvm.compiler.used set.
- collectUsedGlobalVariables(M, Vec, /*CompilerUsed=*/true);
- Used = {llvm::from_range, Vec};
-#endif
- }
-
+ bool ClearDSOLocalOnDeclarations);
void run();
};
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 3d9fb7b12b5d5..50100a63cf407 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -182,6 +182,15 @@ static cl::opt<bool> CtxprofMoveRootsToOwnModule(
"their own module."),
cl::Hidden, cl::init(false));
+cl::list<GlobalValue::GUID> MoveSymbolGUID(
+ "thinlto-move-symbols",
+ cl::desc(
+ "Move the symbols with the given name. This will delete these symbols "
+ "wherever they are originally defined, and make sure their "
+ "linkage is External where they are imported. It is meant to be "
+ "used with the name of contextual profiling roots."),
+ cl::Hidden);
+
namespace llvm {
extern cl::opt<bool> EnableMemProfContextDisambiguation;
}
@@ -1858,6 +1867,15 @@ Expected<bool> FunctionImporter::importFunctions(
LLVM_DEBUG(dbgs() << "Starting import for Module "
<< DestModule.getModuleIdentifier() << "\n");
unsigned ImportedCount = 0, ImportedGVCount = 0;
+ // Before carrying out any imports, see if this module defines functions in
+ // MoveSymbolGUID. If it does, delete them here (but leave the declaration).
+ // The function will be imported elsewhere, as extenal linkage, and the
+ // destination doesn't yet have its definition.
+ DenseSet<GlobalValue::GUID> MoveSymbolGUIDSet;
+ MoveSymbolGUIDSet.insert_range(MoveSymbolGUID);
+ for (auto &F : DestModule)
+ if (!F.isDeclaration() && MoveSymbolGUIDSet.contains(F.getGUID()))
+ F.deleteBody();
IRMover Mover(DestModule);
diff --git a/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp b/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
index ae1af943bc11c..81e461e28df17 100644
--- a/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
+++ b/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
@@ -24,6 +24,31 @@ static cl::opt<bool> UseSourceFilenameForPromotedLocals(
"This requires that the source filename has a unique name / "
"path to avoid name collisions."));
+extern cl::list<GlobalValue::GUID> MoveSymbolGUID;
+
+FunctionImportGlobalProcessing::FunctionImportGlobalProcessing(
+ Module &M, const ModuleSummaryIndex &Index,
+ SetVector<GlobalValue *> *GlobalsToImport, bool ClearDSOLocalOnDeclarations)
+ : M(M), ImportIndex(Index), GlobalsToImport(GlobalsToImport),
+ ClearDSOLocalOnDeclarations(ClearDSOLocalOnDeclarations) {
+ // 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
+ SmallVector<GlobalValue *, 4> Vec;
+ // First collect those in the llvm.used set.
+ collectUsedGlobalVariables(M, Vec, /*CompilerUsed=*/false);
+ // Next collect those in the llvm.compiler.used set.
+ collectUsedGlobalVariables(M, Vec, /*CompilerUsed=*/true);
+ Used = {llvm::from_range, Vec};
+#endif
+ SymbolsToMove.insert_range(MoveSymbolGUID);
+}
+
/// Checks if we should import SGV as a definition, otherwise import as a
/// declaration.
bool FunctionImportGlobalProcessing::doImportAsDefinition(
@@ -147,7 +172,9 @@ FunctionImportGlobalProcessing::getLinkage(const GlobalValue *SGV,
// and/or optimization, but are turned into declarations later
// during the EliminateAvailableExternally pass.
if (doImportAsDefinition(SGV) && !isa<GlobalAlias>(SGV))
- return GlobalValue::AvailableExternallyLinkage;
+ return SymbolsToMove.contains(SGV->getGUID())
+ ? GlobalValue::ExternalLinkage
+ : GlobalValue::AvailableExternallyLinkage;
// An imported external declaration stays external.
return SGV->getLinkage();
diff --git a/llvm/test/ThinLTO/X86/ctxprof-separate-module.ll b/llvm/test/ThinLTO/X86/ctxprof-separate-module.ll
index 391fe21a1b638..b6824a0f9f08c 100644
--- a/llvm/test/ThinLTO/X86/ctxprof-separate-module.ll
+++ b/llvm/test/ThinLTO/X86/ctxprof-separate-module.ll
@@ -22,15 +22,31 @@
; RUN: -r %t/m2.bc,m2_f1,plx \
; RUN: -r %t/m3.bc,m1_f1 \
; RUN: -r %t/m3.bc,m3_f1,plx -debug-only=function-import 2>&1 | FileCheck %s --check-prefix=ABSENT-MSG
+
+; also add the move semantics for the root:
+; RUN: llvm-lto2 run %t/m1.bc %t/m2.bc %t/m3.bc %t/6019442868614718803.bc -thinlto-move-ctxprof-trees \
+; RUN: -thinlto-move-symbols=6019442868614718803 \
+; RUN: -o %t/result-with-move.o -save-temps \
+; RUN: -use-ctx-profile=%t/ctxprof.bitstream \
+; RUN: -r %t/m1.bc,m1_f1,plx \
+; RUN: -r %t/m2.bc,m2_f1,plx \
+; RUN: -r %t/m3.bc,m1_f1 \
+; RUN: -r %t/m3.bc,m3_f1,plx -debug-only=function-import 2>&1 | FileCheck %s --check-prefix=ABSENT-MSG
+
; RUN: llvm-dis %t/result.o.4.3.import.bc -o - | FileCheck %s
; RUN: llvm-dis %t/result.o.3.3.import.bc -o - | FileCheck %s --check-prefix=ABSENT
+; RUN: llvm-dis %t/result-with-move.o.1.3.import.bc -o - | FileCheck %s --check-prefix=WITHMOVE-SRC
+; RUN: llvm-dis %t/result-with-move.o.4.3.import.bc -o - | FileCheck %s --check-prefix=WITHMOVE-DEST
+; RUN: llvm-dis %t/result.o.1.3.import.bc -o - | FileCheck %s --check-prefix=WITHOUTMOVE-SRC
;
-;
-; CHECK: m1_f1()
-; CHECK: m2_f1()
+; CHECK: define available_externally void @m1_f1()
+; CHECK: define available_externally void @m2_f1()
; ABSENT: declare void @m1_f1()
; ABSENT-MSG: Skipping over 6019442868614718803 because its import is handled in a different module.
;
+; WITHMOVE-SRC: declare dso_local void @m1_f1
+; WITHMOVE-DEST: define dso_local void @m1_f1
+; WITHOUTMOVE-SRC: define dso_local void @m1_f1
;--- ctxprof.yaml
Contexts:
-
|
mtrofin
force-pushed
the
users/mtrofin/04-01-_ctxprof_don_t_import_roots_elsewhere
branch
from
357d66a to
627654f
Compare
mtrofin
force-pushed
the
users/mtrofin/04-02-_ctxprof_support_for_move_semantics_for_the_contextual_root
branch
from
f9b3bfa to
e7921cb
Compare
Base automatically changed from
users/mtrofin/04-01-_ctxprof_don_t_import_roots_elsewhere
to
main
April 3, 2025 20:21
mtrofin
force-pushed
the
users/mtrofin/04-02-_ctxprof_support_for_move_semantics_for_the_contextual_root
branch
from
e7921cb to
4eca003
Compare
snehasish
reviewed
Apr 3, 2025
Comment on lines
+41
to
+48
| #ifndef NDEBUG | ||
| SmallVector<GlobalValue *, 4> Vec; | ||
| // First collect those in the llvm.used set. | ||
| collectUsedGlobalVariables(M, Vec, /*CompilerUsed=*/false); | ||
| // Next collect those in the llvm.compiler.used set. | ||
| collectUsedGlobalVariables(M, Vec, /*CompilerUsed=*/true); | ||
| Used = {llvm::from_range, Vec}; | ||
| #endif |
There was a problem hiding this comment.
I don't understand what Used is needed for in debug builds?
There was a problem hiding this comment.
This is code that moved from the .h as-is. I could split it into a NFC, but seemed overkill. To your question, IDK, but wouldn't address it here.
| // destination doesn't yet have its definition. | ||
| DenseSet<GlobalValue::GUID> MoveSymbolGUIDSet; | ||
| MoveSymbolGUIDSet.insert_range(MoveSymbolGUID); | ||
| for (auto &F : DestModule) |
There was a problem hiding this comment.
nit: braces for the loop body.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah since this 2 levels of nesting. I'll be back when I see 3.
snehasish
approved these changes
Apr 3, 2025
| // destination doesn't yet have its definition. | ||
| DenseSet<GlobalValue::GUID> MoveSymbolGUIDSet; | ||
| MoveSymbolGUIDSet.insert_range(MoveSymbolGUID); | ||
| for (auto &F : DestModule) |
There was a problem hiding this comment.
Ah since this 2 levels of nesting. I'll be back when I see 3.
Comment on lines
+41
to
+48
| #ifndef NDEBUG | ||
| SmallVector<GlobalValue *, 4> Vec; | ||
| // First collect those in the llvm.used set. | ||
| collectUsedGlobalVariables(M, Vec, /*CompilerUsed=*/false); | ||
| // Next collect those in the llvm.compiler.used set. | ||
| collectUsedGlobalVariables(M, Vec, /*CompilerUsed=*/true); | ||
| Used = {llvm::from_range, Vec}; | ||
| #endif |
mtrofin
deleted the
users/mtrofin/04-02-_ctxprof_support_for_move_semantics_for_the_contextual_root
branch
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/2804 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/203/builds/6658 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/204/builds/5471 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/138/builds/12338 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/205/builds/5449 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/15759 Here is the relevant piece of the build log for the reference |
mtrofin
added a commit
that referenced
this pull request
Apr 4, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/15748 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
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.
This PR finishes what PR #133992 started.