Skip to content

[ctxprof] Override type of instrumentation if -profile-context-root is specified #128940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

mtrofin
Copy link
Member

@mtrofin mtrofin commented Feb 26, 2025

This patch makes it easy to enable ctxprof instrumentation for targets where the build has a bunch of defaults for instrumented PGO that we want to inherit for ctxprof.

This is switching experimental defaults: we'll eventually enable ctxprof instrumentation through PGOOpt but that type is currently quite entangled and, for the time being, no point adding to that.

Copy link
Member Author

mtrofin commented Feb 26, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@mtrofin mtrofin marked this pull request as ready for review February 26, 2025 20:09
Copy link
Contributor

@teresajohnson teresajohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes looks ok but needs a test.

@mtrofin mtrofin force-pushed the users/mtrofin/02-26-_ctxprof_override_type_of_instrumentation_if_-profile-context-root_is_specified branch from aa7a62f to e773265 Compare February 26, 2025 21:54
@llvmbot llvmbot added PGO Profile Guided Optimizations llvm:transforms labels Feb 26, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 26, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Mircea Trofin (mtrofin)

Changes

This patch makes it easy to enable ctxprof instrumentation for targets where the build has a bunch of defaults for instrumented PGO that we want to inherit for ctxprof.

This is switching experimental defaults: we'll eventually enable ctxprof instrumentation through PGOOpt but that type is currently quite entangled and, for the time being, no point adding to that.


Full diff: https://github.com/llvm/llvm-project/pull/128940.diff

2 Files Affected:

  • (modified) llvm/lib/Passes/PassBuilderPipelines.cpp (+4-4)
  • (added) llvm/test/Transforms/PGOProfile/ctx-instrumentation-optin.ll (+14)
diff --git a/llvm/lib/Passes/PassBuilderPipelines.cpp b/llvm/lib/Passes/PassBuilderPipelines.cpp
index d4c3fe9562f9e..546a5eb1ec283 100644
--- a/llvm/lib/Passes/PassBuilderPipelines.cpp
+++ b/llvm/lib/Passes/PassBuilderPipelines.cpp
@@ -1206,7 +1206,10 @@ PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
 
   // We already asserted this happens in non-FullLTOPostLink earlier.
   const bool IsPreLink = Phase != ThinOrFullLTOPhase::ThinLTOPostLink;
-  const bool IsPGOPreLink = PGOOpt && IsPreLink;
+  // Enable contextual profiling instrumentation.
+  const bool IsCtxProfGen =
+      IsPreLink && PGOCtxProfLoweringPass::isCtxIRPGOInstrEnabled();
+  const bool IsPGOPreLink = !IsCtxProfGen && PGOOpt && IsPreLink;
   const bool IsPGOInstrGen =
       IsPGOPreLink && PGOOpt->Action == PGOOptions::IRInstr;
   const bool IsPGOInstrUse =
@@ -1217,9 +1220,6 @@ PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
   assert(!(IsPGOInstrGen && PGOCtxProfLoweringPass::isCtxIRPGOInstrEnabled()) &&
          "Enabling both instrumented PGO and contextual instrumentation is not "
          "supported.");
-  // Enable contextual profiling instrumentation.
-  const bool IsCtxProfGen = !IsPGOInstrGen && IsPreLink &&
-                            PGOCtxProfLoweringPass::isCtxIRPGOInstrEnabled();
   const bool IsCtxProfUse =
       !UseCtxProfile.empty() && Phase == ThinOrFullLTOPhase::ThinLTOPreLink;
 
diff --git a/llvm/test/Transforms/PGOProfile/ctx-instrumentation-optin.ll b/llvm/test/Transforms/PGOProfile/ctx-instrumentation-optin.ll
new file mode 100644
index 0000000000000..5231007803433
--- /dev/null
+++ b/llvm/test/Transforms/PGOProfile/ctx-instrumentation-optin.ll
@@ -0,0 +1,14 @@
+; RUN: opt -O2 -debug-pass-manager -S -pgo-kind=pgo-instr-gen-pipeline -profile-file='temp' \
+; RUN:  < %s  2>&1 | FileCheck %s --check-prefixes=COMMON,PGO
+; RUN: opt -O2 -debug-pass-manager -S -pgo-kind=pgo-instr-gen-pipeline -profile-file='temp' \
+; RUN:  -profile-context-root=something < %s 2>&1 | FileCheck %s --check-prefixes=COMMON,CTXPROF
+
+; COMMON:   Running pass: PGOInstrumentationGen
+; COMMON:   Invalidating analysis: InnerAnalysisManagerProxy<FunctionAnalysisManager, Module>
+; COMMON:   Invalidating analysis: LazyCallGraphAnalysis
+; COMMON:   Invalidating analysis: InnerAnalysisManagerProxy<CGSCCAnalysisManager, Module>
+; CTXPROF:  Running pass: AssignGUIDPass
+; CTXPROF:  Running pass: NoinlineNonPrevailing on [module]
+; COMMON:   Running analysis: InnerAnalysisManagerProxy<FunctionAnalysisManager, Module>
+; PGO:      Running pass: InstrProfilingLoweringPass
+; CTXPROF:  Running pass: PGOCtxProfLoweringPass on [module]
\ No newline at end of file

@llvmbot
Copy link
Member

llvmbot commented Feb 26, 2025

@llvm/pr-subscribers-pgo

Author: Mircea Trofin (mtrofin)

Changes

This patch makes it easy to enable ctxprof instrumentation for targets where the build has a bunch of defaults for instrumented PGO that we want to inherit for ctxprof.

This is switching experimental defaults: we'll eventually enable ctxprof instrumentation through PGOOpt but that type is currently quite entangled and, for the time being, no point adding to that.


Full diff: https://github.com/llvm/llvm-project/pull/128940.diff

2 Files Affected:

  • (modified) llvm/lib/Passes/PassBuilderPipelines.cpp (+4-4)
  • (added) llvm/test/Transforms/PGOProfile/ctx-instrumentation-optin.ll (+14)
diff --git a/llvm/lib/Passes/PassBuilderPipelines.cpp b/llvm/lib/Passes/PassBuilderPipelines.cpp
index d4c3fe9562f9e..546a5eb1ec283 100644
--- a/llvm/lib/Passes/PassBuilderPipelines.cpp
+++ b/llvm/lib/Passes/PassBuilderPipelines.cpp
@@ -1206,7 +1206,10 @@ PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
 
   // We already asserted this happens in non-FullLTOPostLink earlier.
   const bool IsPreLink = Phase != ThinOrFullLTOPhase::ThinLTOPostLink;
-  const bool IsPGOPreLink = PGOOpt && IsPreLink;
+  // Enable contextual profiling instrumentation.
+  const bool IsCtxProfGen =
+      IsPreLink && PGOCtxProfLoweringPass::isCtxIRPGOInstrEnabled();
+  const bool IsPGOPreLink = !IsCtxProfGen && PGOOpt && IsPreLink;
   const bool IsPGOInstrGen =
       IsPGOPreLink && PGOOpt->Action == PGOOptions::IRInstr;
   const bool IsPGOInstrUse =
@@ -1217,9 +1220,6 @@ PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
   assert(!(IsPGOInstrGen && PGOCtxProfLoweringPass::isCtxIRPGOInstrEnabled()) &&
          "Enabling both instrumented PGO and contextual instrumentation is not "
          "supported.");
-  // Enable contextual profiling instrumentation.
-  const bool IsCtxProfGen = !IsPGOInstrGen && IsPreLink &&
-                            PGOCtxProfLoweringPass::isCtxIRPGOInstrEnabled();
   const bool IsCtxProfUse =
       !UseCtxProfile.empty() && Phase == ThinOrFullLTOPhase::ThinLTOPreLink;
 
diff --git a/llvm/test/Transforms/PGOProfile/ctx-instrumentation-optin.ll b/llvm/test/Transforms/PGOProfile/ctx-instrumentation-optin.ll
new file mode 100644
index 0000000000000..5231007803433
--- /dev/null
+++ b/llvm/test/Transforms/PGOProfile/ctx-instrumentation-optin.ll
@@ -0,0 +1,14 @@
+; RUN: opt -O2 -debug-pass-manager -S -pgo-kind=pgo-instr-gen-pipeline -profile-file='temp' \
+; RUN:  < %s  2>&1 | FileCheck %s --check-prefixes=COMMON,PGO
+; RUN: opt -O2 -debug-pass-manager -S -pgo-kind=pgo-instr-gen-pipeline -profile-file='temp' \
+; RUN:  -profile-context-root=something < %s 2>&1 | FileCheck %s --check-prefixes=COMMON,CTXPROF
+
+; COMMON:   Running pass: PGOInstrumentationGen
+; COMMON:   Invalidating analysis: InnerAnalysisManagerProxy<FunctionAnalysisManager, Module>
+; COMMON:   Invalidating analysis: LazyCallGraphAnalysis
+; COMMON:   Invalidating analysis: InnerAnalysisManagerProxy<CGSCCAnalysisManager, Module>
+; CTXPROF:  Running pass: AssignGUIDPass
+; CTXPROF:  Running pass: NoinlineNonPrevailing on [module]
+; COMMON:   Running analysis: InnerAnalysisManagerProxy<FunctionAnalysisManager, Module>
+; PGO:      Running pass: InstrProfilingLoweringPass
+; CTXPROF:  Running pass: PGOCtxProfLoweringPass on [module]
\ No newline at end of file

@mtrofin
Copy link
Member Author

mtrofin commented Feb 26, 2025

Changes looks ok but needs a test.

Yup, done.

@mtrofin mtrofin force-pushed the users/mtrofin/02-26-_ctxprof_override_type_of_instrumentation_if_-profile-context-root_is_specified branch from e773265 to 68811b9 Compare February 26, 2025 21:58
Copy link
Contributor

@teresajohnson teresajohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mtrofin mtrofin force-pushed the users/mtrofin/02-26-_ctxprof_override_type_of_instrumentation_if_-profile-context-root_is_specified branch from 68811b9 to 14a6958 Compare February 27, 2025 01:00
Copy link
Member Author

mtrofin commented Feb 27, 2025

Merge activity

  • Feb 26, 10:55 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Feb 26, 10:57 PM EST: A user merged this pull request with Graphite.

@mtrofin mtrofin merged commit eb1c3ac into main Feb 27, 2025
11 checks passed
@mtrofin mtrofin deleted the users/mtrofin/02-26-_ctxprof_override_type_of_instrumentation_if_-profile-context-root_is_specified branch February 27, 2025 03:57
@llvm-ci
Copy link
Collaborator

llvm-ci commented Feb 27, 2025

LLVM Buildbot has detected a new failure on builder clang-debian-cpp20 running on clang-debian-cpp20 while building llvm at step 2 "checkout".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/108/builds/9775

Here is the relevant piece of the build log for the reference
Step 2 (checkout) failure: update (failure)
git version 2.43.0
fatal: unable to access 'https://github.com/llvm/llvm-project.git/': Could not resolve host: github.com
fatal: unable to access 'https://github.com/llvm/llvm-project.git/': Could not resolve host: github.com

joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Mar 3, 2025
… is specified (llvm#128940)

This patch makes it easy to enable ctxprof instrumentation for targets where the build has a bunch of defaults for instrumented PGO that we want to inherit for ctxprof.

This is switching experimental defaults: we'll eventually enable ctxprof instrumentation through `PGOOpt` but that type is currently quite entangled and, for the time being, no point adding to that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:transforms PGO Profile Guided Optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants