[PATCH] D50591: [PGO] Control Height Reduction

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:362
+
+  void setPerScopeBias(SmallVectorImpl<CHRScope *> &Scopes);
+  void setPerScopeBias(CHRScope *Scope, CHRScope *OutermostScope);
----------------
maybe rename it to 'classifyBiasedScopes' -- which means classify them into true or false biased regions?


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:588
+// MD_prof metadata if it's well-formed.
+static bool CheckMDProf(MDNode *MD, double &TrueProb, double &FalseProb) {
+  if (!MD) return false;
----------------
Through out the patch, 'double' is used for branch probability type. Please use existing fixed point representation BranchProbability type in Support/


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:1727
+  OldBR->dropAllReferences();
+  BranchInst *NewBR = BranchInst::Create(NewEntryBlock,
+                                         cast<BasicBlock>(VMap[NewEntryBlock]),
----------------
Is is possible for this be 'folded' into unconditional branch given the true predicate?


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:1730
+                                         ConstantInt::getTrue(F.getContext()));
+  PreEntryBlock->getInstList().push_back(NewBR);
+  assert(NewEntryBlock->getSinglePredecessor() == EntryBlock &&
----------------
Is it better to use IRBuilder interface here?


Repository:
  rL LLVM

https://reviews.llvm.org/D50591