-
Notifications
You must be signed in to change notification settings - Fork 11.5k
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
[ctx_profile] Decouple ctx instrumentation from PGOOpt #92445
Conversation
We currently don't support passing files and don't need frontend involvment either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm but with a couple of minor comments/suggestions
@@ -1157,8 +1153,12 @@ PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level, | |||
const bool IsPGOInstrUse = | |||
IsPGOPreLink && PGOOpt->Action == PGOOptions::IRUse; | |||
const bool IsMemprofUse = IsPGOPreLink && !PGOOpt->MemoryProfile.empty(); | |||
// We don't want to mix pgo ctx gen and pgo gen; we also don't currently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be an assert that fires if both are enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@@ -1168,9 +1168,13 @@ PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level, | |||
/*IsCS=*/false, PGOOpt->AtomicCounterUpdate, | |||
PGOOpt->ProfileFile, PGOOpt->ProfileRemappingFile, | |||
PGOOpt->FS); | |||
} else if (IsCtxProfGen) { | |||
MPM.addPass(PGOInstrumentationGen(false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document constant parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
…1da5ad733 Local branch amd-gfx 3c31da5 Merged main:911207338add39b8439a09fd7f9d97bbe4a335bd into amd-gfx:3ae085055ecb Remote branch main d311a62 [ctx_profile] Decouple ctx instrumentation from PGOOpt (llvm#92445)
We currently don't support passing files and don't need frontend involvment either.