[PATCH] D50591: [PGO] Control Height Reduction

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Passes/PassBuilder.cpp:198
+static cl::opt<bool>
+    EnableCHR("enable-chr-npm", cl::init(true), cl::Hidden,
+              cl::desc("Enable CHR"));
----------------
what is npm?


================
Comment at: lib/Passes/PassBuilder.cpp:199
+    EnableCHR("enable-chr-npm", cl::init(true), cl::Hidden,
+              cl::desc("Enable CHR"));
+
----------------
Enable CHR --> Enable control height reduction optimization (CHR)


================
Comment at: lib/Passes/PassBuilder.cpp:494
 
+  if (EnableCHR && Level == O3 && PGOOpt && !PGOOpt->ProfileUseFile.empty())
+    FPM.addPass(ControlHeightReductionPass());
----------------
How about SamplePGO ?


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:388
+                                        nullptr, nullptr, &Status);
+    return CHRModules.count(F.getParent()->getName()) ||
+        CHRFunctions.count(Name) ||
----------------
CHRModules should be checked before function name demangling and check.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:393
+
+  assert(PSI.hasProfileSummary() && "Empty PSI?");
+  return PSI.isFunctionEntryHot(&F);
----------------
For stress testing purpose, perhaps adding a force-chr option so that it is applied on cold functions as well?


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:440
+
+static bool isHoistableInstructionType(Instruction *I) {
+  return isa<BinaryOperator>(I) || isa<CastInst>(I) || isa<SelectInst>(I) ||
----------------
need documentation comment for the function.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:534
+// If a branch or select is biased more than this ratio, it's considered biased.
+static constexpr double BiasThreshold = 0.99;
+
----------------
Make it an internal option.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:537
+// Returns true and sets the true probability and false probability of an
+// MD_prof metadata if it's well-formed and biased.
+static bool CheckBiasedMDProf(MDNode *MD, double &TrueProb, double &FalseProb) {
----------------
the comment does not match the implementation: it returns true as long as MD is well formed.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:595
+// select is biased.
+static bool CheckBiasedSelect(
+    SelectInst *SI, Region *R,
----------------
Can some refactoriing be done to share some of the implementation with CheckBiasedBranch?


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:657
+
+CHRScope * CHR::findScopes(Region *R, Region *NextRegion, Region *ParentRegion,
+                           SmallVectorImpl<CHRScope *> &Scopes) {
----------------
This method looks pretty big. Can it be broken up?


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:1378
+
+void CHR::transformScopes(CHRScope *Scope, DenseSet<PHINode *> &TrivialPHIs) {
+  CHR_DEBUG(dbgs() << "transformScopes " << *Scope << "\n");
----------------
this method is also gigantic. Suggest breaking it up (the debug check code can also be put into separate helpers).


Repository:
  rL LLVM

https://reviews.llvm.org/D50591