[ctx_prof] Fix the pre-thinlink "use" case #102511
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
Didn't notice in llvm#101338 that the instrumentation in `llvm/test/Transforms/PGOProfile/ctx-prof-use-prelink.ll` was actually incorrect.
|
@llvm/pr-subscribers-pgo @llvm/pr-subscribers-llvm-transforms Author: Mircea Trofin (mtrofin) ChangesDidn't notice in #101338 that the instrumentation in Full diff: https://github.com/llvm/llvm-project/pull/102511.diff 5 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Instrumentation/PGOCtxProfLowering.h b/llvm/include/llvm/Transforms/Instrumentation/PGOCtxProfLowering.h
index 5256aff56205ba..f127d16b8de124 100644
--- a/llvm/include/llvm/Transforms/Instrumentation/PGOCtxProfLowering.h
+++ b/llvm/include/llvm/Transforms/Instrumentation/PGOCtxProfLowering.h
@@ -19,7 +19,8 @@ class Type;
class PGOCtxProfLoweringPass : public PassInfoMixin<PGOCtxProfLoweringPass> {
public:
explicit PGOCtxProfLoweringPass() = default;
- static bool isContextualIRPGOEnabled();
+ // True if contextual instrumentation is enabled.
+ static bool isCtxIRPGOInstrEnabled();
PreservedAnalyses run(Module &M, ModuleAnalysisManager &MAM);
};
diff --git a/llvm/lib/Passes/PassBuilderPipelines.cpp b/llvm/lib/Passes/PassBuilderPipelines.cpp
index c175ee89809849..9ac84ed5dd5178 100644
--- a/llvm/lib/Passes/PassBuilderPipelines.cpp
+++ b/llvm/lib/Passes/PassBuilderPipelines.cpp
@@ -1173,13 +1173,12 @@ PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
const bool IsMemprofUse = IsPGOPreLink && !PGOOpt->MemoryProfile.empty();
// We don't want to mix pgo ctx gen and pgo gen; we also don't currently
// enable ctx profiling from the frontend.
- assert(
- !(IsPGOInstrGen && PGOCtxProfLoweringPass::isContextualIRPGOEnabled()) &&
- "Enabling both instrumented FDO and contextual instrumentation is not "
- "supported.");
+ assert(!(IsPGOInstrGen && PGOCtxProfLoweringPass::isCtxIRPGOInstrEnabled()) &&
+ "Enabling both instrumented FDO and contextual instrumentation is not "
+ "supported.");
// Enable contextual profiling instrumentation.
const bool IsCtxProfGen = !IsPGOInstrGen && IsPreLink &&
- PGOCtxProfLoweringPass::isContextualIRPGOEnabled();
+ PGOCtxProfLoweringPass::isCtxIRPGOInstrEnabled();
const bool IsCtxProfUse = !UseCtxProfile.empty() && !PGOOpt &&
Phase == ThinOrFullLTOPhase::ThinLTOPreLink;
@@ -1670,8 +1669,10 @@ PassBuilder::buildThinLTOPreLinkDefaultPipeline(OptimizationLevel Level) {
// In pre-link, for ctx prof use, we stop here with an instrumented IR. We let
// thinlto use the contextual info to perform imports; then use the contextual
// profile in the post-thinlink phase.
- if (!UseCtxProfile.empty() && !PGOOpt)
+ if (!UseCtxProfile.empty() && !PGOOpt) {
+ addRequiredLTOPreLinkPasses(MPM);
return MPM;
+ }
// Run partial inlining pass to partially inline functions that have
// large bodies.
diff --git a/llvm/lib/Transforms/Instrumentation/PGOCtxProfLowering.cpp b/llvm/lib/Transforms/Instrumentation/PGOCtxProfLowering.cpp
index de1d4d2381c06e..d6ba12465bb328 100644
--- a/llvm/lib/Transforms/Instrumentation/PGOCtxProfLowering.cpp
+++ b/llvm/lib/Transforms/Instrumentation/PGOCtxProfLowering.cpp
@@ -30,7 +30,7 @@ static cl::list<std::string> ContextRoots(
"root of an interesting graph, which will be profiled independently "
"from other similar graphs."));
-bool PGOCtxProfLoweringPass::isContextualIRPGOEnabled() {
+bool PGOCtxProfLoweringPass::isCtxIRPGOInstrEnabled() {
return !ContextRoots.empty();
}
diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
index 1ce8f58c1aa140..41618194d12ed7 100644
--- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -321,6 +321,7 @@ static cl::opt<unsigned> PGOFunctionCriticalEdgeThreshold(
" greater than this threshold."));
extern cl::opt<unsigned> MaxNumVTableAnnotations;
+extern cl::opt<std::string> UseCtxProfile;
namespace llvm {
// Command line option to turn on CFG dot dump after profile annotation.
@@ -338,9 +339,12 @@ extern cl::opt<bool> EnableVTableProfileUse;
extern cl::opt<InstrProfCorrelator::ProfCorrelatorKind> ProfileCorrelate;
} // namespace llvm
+bool shouldInstrumentForCtxProf() {
+ return PGOCtxProfLoweringPass::isCtxIRPGOInstrEnabled() ||
+ !UseCtxProfile.empty();
+}
bool shouldInstrumentEntryBB() {
- return PGOInstrumentEntry ||
- PGOCtxProfLoweringPass::isContextualIRPGOEnabled();
+ return PGOInstrumentEntry || shouldInstrumentForCtxProf();
}
// FIXME(mtrofin): re-enable this for ctx profiling, for non-indirect calls. Ctx
@@ -348,8 +352,7 @@ bool shouldInstrumentEntryBB() {
// Supporting other values is relatively straight-forward - just another counter
// range within the context.
bool isValueProfilingDisabled() {
- return DisableValueProfiling ||
- PGOCtxProfLoweringPass::isContextualIRPGOEnabled();
+ return DisableValueProfiling || shouldInstrumentForCtxProf();
}
// Return a string describing the branch condition that can be
@@ -902,7 +905,7 @@ static void instrumentOneFunc(
unsigned NumCounters =
InstrumentBBs.size() + FuncInfo.SIVisitor.getNumOfSelectInsts();
- if (PGOCtxProfLoweringPass::isContextualIRPGOEnabled()) {
+ if (shouldInstrumentForCtxProf()) {
auto *CSIntrinsic =
Intrinsic::getDeclaration(M, Intrinsic::instrprof_callsite);
// We want to count the instrumentable callsites, then instrument them. This
@@ -1861,7 +1864,7 @@ static bool InstrumentAllFunctions(
function_ref<BlockFrequencyInfo *(Function &)> LookupBFI, bool IsCS) {
// For the context-sensitve instrumentation, we should have a separated pass
// (before LTO/ThinLTO linking) to create these variables.
- if (!IsCS && !PGOCtxProfLoweringPass::isContextualIRPGOEnabled())
+ if (!IsCS && !shouldInstrumentForCtxProf())
createIRLevelProfileFlagVar(M, /*IsCS=*/false);
Triple TT(M.getTargetTriple());
@@ -2112,7 +2115,7 @@ static bool annotateAllFunctions(
bool InstrumentFuncEntry = PGOReader->instrEntryBBEnabled();
if (PGOInstrumentEntry.getNumOccurrences() > 0)
InstrumentFuncEntry = PGOInstrumentEntry;
- InstrumentFuncEntry |= PGOCtxProfLoweringPass::isContextualIRPGOEnabled();
+ InstrumentFuncEntry |= shouldInstrumentForCtxProf();
bool HasSingleByteCoverage = PGOReader->hasSingleByteCoverage();
for (auto &F : M) {
diff --git a/llvm/test/Transforms/PGOProfile/ctx-prof-use-prelink.ll b/llvm/test/Transforms/PGOProfile/ctx-prof-use-prelink.ll
index b50a815be5abf5..18ac2f92aa39d4 100644
--- a/llvm/test/Transforms/PGOProfile/ctx-prof-use-prelink.ll
+++ b/llvm/test/Transforms/PGOProfile/ctx-prof-use-prelink.ll
@@ -1,4 +1,4 @@
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals all --version 5
; There is no profile, but that's OK because the prelink does not care about
; the content of the profile, just that we intend to use one.
; There is no scenario currently of doing ctx profile use without thinlto.
@@ -7,19 +7,22 @@
declare void @bar()
+;.
+; CHECK: @__profn_foo = private constant [3 x i8] c"foo"
+;.
define void @foo(i32 %a, ptr %fct) {
; CHECK-LABEL: define void @foo(
; CHECK-SAME: i32 [[A:%.*]], ptr [[FCT:%.*]]) local_unnamed_addr {
+; CHECK-NEXT: call void @llvm.instrprof.increment(ptr @__profn_foo, i64 728453322856651412, i32 2, i32 0)
; CHECK-NEXT: [[T:%.*]] = icmp eq i32 [[A]], 0
; CHECK-NEXT: br i1 [[T]], label %[[YES:.*]], label %[[NO:.*]]
; CHECK: [[YES]]:
; CHECK-NEXT: call void @llvm.instrprof.increment(ptr @__profn_foo, i64 728453322856651412, i32 2, i32 1)
-; CHECK-NEXT: [[TMP1:%.*]] = ptrtoint ptr [[FCT]] to i64
-; CHECK-NEXT: call void @llvm.instrprof.value.profile(ptr @__profn_foo, i64 728453322856651412, i64 [[TMP1]], i32 0, i32 0)
+; CHECK-NEXT: call void @llvm.instrprof.callsite(ptr @__profn_foo, i64 728453322856651412, i32 2, i32 0, ptr [[FCT]])
; CHECK-NEXT: call void [[FCT]](i32 0)
; CHECK-NEXT: br label %[[EXIT:.*]]
; CHECK: [[NO]]:
-; CHECK-NEXT: call void @llvm.instrprof.increment(ptr @__profn_foo, i64 728453322856651412, i32 2, i32 0)
+; CHECK-NEXT: call void @llvm.instrprof.callsite(ptr @__profn_foo, i64 728453322856651412, i32 2, i32 1, ptr @bar)
; CHECK-NEXT: call void @bar()
; CHECK-NEXT: br label %[[EXIT]]
; CHECK: [[EXIT]]:
@@ -36,3 +39,6 @@ no:
exit:
ret void
}
+;.
+; CHECK: attributes #[[ATTR0:[0-9]+]] = { nounwind }
+;.
|
boomanaiden154
approved these changes
Aug 8, 2024
snehasish
reviewed
Aug 8, 2024
| define void @foo(i32 %a, ptr %fct) { | ||
| ; CHECK-LABEL: define void @foo( | ||
| ; CHECK-SAME: i32 [[A:%.*]], ptr [[FCT:%.*]]) local_unnamed_addr { | ||
| ; CHECK-NEXT: call void @llvm.instrprof.increment(ptr @__profn_foo, i64 728453322856651412, i32 2, i32 0) |
There was a problem hiding this comment.
Why did this get hoisted out from the no: block? The only functional change I could find in this patch was addLTOPrelinkPasses(MPM) which adds CanonicalizeAliasesPass and NameAnonGlobalPass.
There was a problem hiding this comment.
it's the changes in PGOInstrumentation that do that.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/89/builds/3722 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/180/builds/3001 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/3003 Here is the relevant piece of the build log for the reference: |
|
This patch broke at least one flang build bot (https://lab.llvm.org/buildbot/#/builders/89/builds/3722); please repair or revert with dispatch. |
boomanaiden154
added a commit
that referenced
this pull request
Aug 8, 2024
This reverts commit 1a6d60e. Broke some buildbots.
mtrofin
added a commit
that referenced
this pull request
Aug 9, 2024
This reverts commit 967185e. The problem was link dependencies, moved `UseCtxProfile` to `Analysis`.
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.
Didn't notice in #101338 that the instrumentation in
llvm/test/Transforms/PGOProfile/ctx-prof-use-prelink.llwas actually incorrect.