[PATCH] D54175: [PGO] context sensitive PGO
Teresa Johnson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 16 13:47:42 PST 2019
tejohnson added a comment.
Bunch of mostly nit suggestions about names and commenting, and a few questions. I like the improved profile action expression.
================
Comment at: CMakeLists.txt:575
+ endif()
+ file(TO_NATIVE_PATH "${LLVM_CSPROFILE_DATA_DIR}/%${LLVM_PROFILE_MERGE_POOL_SIZE}m.profraw" LLVM_CSPROFILE_FILE_PATTERN)
+ endif()
----------------
nit: indentation seems off
================
Comment at: cmake/modules/HandleLLVMOptions.cmake:816
+# Nedd to pass -fprofile-instr-use to backend compiler after ld-plugin.
+# This is for context-sensitive PGO compilation.
----------------
s/Nedd/Need
Also, I don't understand how the comment about needing to pass it to the backend compiler after ld-plugin relates to the handling below.
================
Comment at: cmake/modules/HandleLLVMOptions.cmake:826
+
+option(LLVM_USE_NEWPM "Build LLVM using experimental new pass manager" Off)
+mark_as_advanced(LLVM_USE_NEWPM)
----------------
This part seems unrelated - probably split into a different patch?
================
Comment at: include/llvm/Passes/PassBuilder.h:35
struct PGOOptions {
- PGOOptions(std::string ProfileGenFile = "", std::string ProfileUseFile = "",
- std::string SampleProfileFile = "",
- std::string ProfileRemappingFile = "",
- bool RunProfileGen = false, bool SamplePGOSupport = false)
- : ProfileGenFile(ProfileGenFile), ProfileUseFile(ProfileUseFile),
- SampleProfileFile(SampleProfileFile),
- ProfileRemappingFile(ProfileRemappingFile),
- RunProfileGen(RunProfileGen),
- SamplePGOSupport(SamplePGOSupport || !SampleProfileFile.empty()) {
- assert((RunProfileGen ||
- !SampleProfileFile.empty() ||
- !ProfileUseFile.empty() ||
- SamplePGOSupport) && "Illegal PGOOptions.");
- }
- std::string ProfileGenFile;
- std::string ProfileUseFile;
- std::string SampleProfileFile;
+ enum PGOAction { NoAction, IRInstr, IRUse, SampleUse };
+ enum CSPGOAction { NoCSAction, CSIRInstr, CSIRUse };
----------------
document different values (in both enums)
================
Comment at: include/llvm/Passes/PassBuilder.h:45
+ // We do the following checks:
+ // (1) Action == NoAction && CSAction == NOCSAction && SamplePGOSupport,
+ // (2) if Action != NoAction then !ProfileFile.empty(),
----------------
This ordering doesn't seem to match the ordering of the checks in the assert below. In any case, rather than have the documentation just duplicate the code (it doesn't really add anything this way), it would be better to split up the following assert into multiple, with more of a plain english explanation of each one.
================
Comment at: include/llvm/ProfileData/InstrProf.h:770
uint64_t Hash;
+ static const int CS_FLAG_IN_FUNC_HASH = 60;
----------------
document
================
Comment at: include/llvm/ProfileData/InstrProf.h:1016
// raw header.
+// Version 5: Bit 60-63 of FuncHash is reserved for storing other information.
const uint64_t Version = INSTR_PROF_RAW_VERSION;
----------------
this is just the addition of bit 60 being the CS flag, right? Maybe just say that?
================
Comment at: include/llvm/ProfileData/InstrProfReader.h:438
const unsigned char *readSummary(IndexedInstrProf::ProfVersion Version,
- const unsigned char *Cur);
+ const unsigned char *Cur, bool IsCS);
----------------
Document new parameter here and elsewhere. I assume it is legal to have both a CS and a non-CS summary in this object? In that case, maybe the parameter name should be something like UseCS.
================
Comment at: include/llvm/Transforms/Instrumentation/InstrProfiling.h:64
size_t NamesSize;
+ bool IsCS;
----------------
document variable
================
Comment at: include/llvm/Transforms/Instrumentation/PGOInstrumentation.h:39
+ createProfileNameVar(M, CSInstrName);
+ createIRLevelProfileFlagVar(M, true);
+ return PreservedAnalyses::all();
----------------
document const parameter
================
Comment at: lib/Passes/PassBuilder.cpp:523
// at -Os/Oz.
- if (!isOptimizingForSize(Level)) {
+ if (!isOptimizingForSize(Level) && !IsCS) {
InlineParams IP;
----------------
Why do we skip size optimizations with CS?
================
Comment at: lib/Passes/PassBuilder.cpp:777
+ if (PGOOpt->CSAction == PGOOptions::CSIRInstr)
+ addPGOInstrPasses(MPM, DebugLogging, Level, true, /* IsCS */ true,
+ PGOOpt->CSProfileGenFile, PGOOpt->ProfileRemappingFile);
----------------
document const parameter (here and in below call)
================
Comment at: lib/Passes/PassBuilder.cpp:1022
// FIXME: We should use a customized pre-link pipeline!
- return buildPerModuleDefaultPipeline(Level, DebugLogging);
+ return buildPerModuleDefaultPipeline(Level, DebugLogging, true);
}
----------------
document const parameter
================
Comment at: lib/Passes/PassBuilder.cpp:1145
+ if (PGOOpt->CSAction == PGOOptions::CSIRInstr)
+ addPGOInstrPasses(MPM, DebugLogging, Level, true, /* IsCS */ true,
+ PGOOpt->CSProfileGenFile, PGOOpt->ProfileRemappingFile);
----------------
document const parameter (here and below call)
================
Comment at: lib/ProfileData/InstrProf.cpp:1045
+ Triple TT(M.getTargetTriple());
+ if (!TT.supportsCOMDAT())
+ IRLevelVersionVariable->setLinkage(GlobalValue::WeakAnyLinkage);
----------------
nit - in createProfileNameVar the linkage initially set to weak any, then reset to external if supports comdat. Here you do the opposite - suggest making the logic the same in both places for consistency
================
Comment at: lib/ProfileData/InstrProfReader.cpp:815
- Cur = readSummary((IndexedInstrProf::ProfVersion)FormatVersion, Cur);
+ Cur = readSummary((IndexedInstrProf::ProfVersion)FormatVersion, Cur, false);
+ if (Header->Version & VARIANT_MASK_CSIR_PROF)
----------------
document const parameter here and in below call
================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:537
+ // Create proifle COMDAT variables. We need to do this before
+ // LTO/Thin-lto, otherwise lld will not work.
----------------
s/proifle/profile/
================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:538
+ // Create proifle COMDAT variables. We need to do this before
+ // LTO/Thin-lto, otherwise lld will not work.
+ if (!PerformThinLTO && EnablePGOCSInstrGen)
----------------
Can you add a better explanation about linker issue - i.e. the linker wants to see all variables before the LTO link since it needs to resolve symbols/comdats.
================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:575
+ if (!(PrepareForLTO || PrepareForThinLTO))
+ addPGOInstrPasses(MPM, true);
+
----------------
document const parameter (here and below)
================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:276
+ if (PreheaderCount &&
+ (PreheaderCount.getValue() * 3) >= (InstrCount.getValue() * 2))
+ continue;
----------------
document rationale for this equation
================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:998
void InstrProfiling::emitInitialization() {
- StringRef InstrProfileOutput = Options.InstrProfileOutput;
-
- if (!InstrProfileOutput.empty()) {
- // Create variable for profile name.
- Constant *ProfileNameConst =
- ConstantDataArray::getString(M->getContext(), InstrProfileOutput, true);
- GlobalVariable *ProfileNameVar = new GlobalVariable(
- *M, ProfileNameConst->getType(), true, GlobalValue::WeakAnyLinkage,
- ProfileNameConst, INSTR_PROF_QUOTE(INSTR_PROF_PROFILE_NAME_VAR));
- if (TT.supportsCOMDAT()) {
- ProfileNameVar->setLinkage(GlobalValue::ExternalLinkage);
- ProfileNameVar->setComdat(M->getOrInsertComdat(
- StringRef(INSTR_PROF_QUOTE(INSTR_PROF_PROFILE_NAME_VAR))));
- }
+ if (!IsCS) {
+ StringRef InstrProfileOutput = Options.InstrProfileOutput;
----------------
Comment needed
================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:410
private:
+ bool IsCS;
bool runOnModule(Module &M) override;
----------------
document
================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:435
std::string ProfileFileName;
+ bool IsCS;
----------------
document
================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:554
Function &F;
+ bool IsCS;
----------------
document
================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:666
+ // Hash format for context senstive profile. Reserve 4 bits for other
+ // information.
FunctionHash = (uint64_t)SIVisitor.getNumOfSelectInsts() << 56 |
----------------
Probably needs a slight expansion on why that other info shouldn't affect function hash.
================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:678
<< ValueSites[IPVK_IndirectCallTarget].size()
+ << ", Selects = " << SIVisitor.getNumOfSelectInsts()
<< ", Hash = " << FunctionHash << "\n";);
----------------
why reorder this?
================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1459
+ if (!IsCS)
+ createIRLevelProfileFlagVar(M, false);
std::unordered_multimap<Comdat *, GlobalValue *> ComdatMembers;
----------------
document const parameter. also add comment for why this is not needed when IsCS
================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1507
+ function_ref<BlockFrequencyInfo *(Function &)> LookupBFI,
+ ProfileSummaryInfo *PSI, bool IsCS) {
LLVM_DEBUG(dbgs() << "Read in profile counters: ");
----------------
I don't see where the new parameter PSI is getting used.
================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1642
+ ProfileSummaryInfo *PSI = nullptr;
+ if (IsCS)
----------------
Here and below, needs comment about why PSI when IsCS. But see comment in annotateAllFunctions - I don't see it getting used there?
================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:716
+ bool FuncIsCS = NamedInstrProfRecord::hasCSFlagInHash(Func.Hash);
+ if (FuncIsCS && !ShowCS)
+ continue;
----------------
How about replace these 4 lines with just:
if (FuncIsCS != ShowCS)
continue;
================
Comment at: tools/opt/NewPMDriver.cpp:116
+
+enum CSPGOKind { NoCSPGO, CSInstrGen, CSInstrUse };
+static cl::opt<CSPGOKind> CSPGOKindFlag(
----------------
Can you do something similar to D56749 (moving and consolidating these opts between PMs)?
================
Comment at: tools/opt/NewPMDriver.cpp:249
+ // TODO: handle CSFDO options.
Optional<PGOOptions> P;
----------------
Is this still TODO? I see handling further below.
================
Comment at: tools/opt/NewPMDriver.cpp:284
+ if (!P)
+ errs() << "CSInstrUse needs to be together with InstrUse";
+ P->CSAction = PGOOptions::CSIRUse;
----------------
As an aside - theoretically someone might want to do CS profiling without regular PGO. Is it worth supporting that combination?
================
Comment at: unittests/ProfileData/InstrProfTest.cpp:179
};
- ProfileSummary &PS = Reader->getSummary();
+ ProfileSummary &PS = Reader->getSummary(false);
VerifySummary(PS);
----------------
document const param
================
Comment at: unittests/ProfileData/InstrProfTest.cpp:805
- ASSERT_EQ(1ULL << 63, Reader->getMaximumFunctionCount());
+ ASSERT_EQ(1ULL << 63, Reader->getMaximumFunctionCount(false));
}
----------------
ditto
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54175/new/
https://reviews.llvm.org/D54175
More information about the llvm-commits
mailing list