[PATCH] D50591: [PGO] Control Height Reduction

[Hiroshi Yamauchi via Phabricator via llvm-commits]

yamauchi added inline comments.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:752
+      CHR_DEBUG(dbgs() << "S1 " << S1->getName() << "\n");
+      if (S0 != S1 && (S0 == Exit || S1 == Exit)) {
+        RegInfo RI(R);
----------------
davidxl wrote:
> is it possible to have S0 == S1?
I think it's unlikely but possible because we could have a branch instruction that has the same label for the true and the false targets.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:828
+// For example, for the following scope/region with selects, we want to insert
+// the merged branch right before the first select in the first/entry block by
+// hoisting c1, c2, c3, and c4.
----------------
davidxl wrote:
> In this example, if the selects are after if (c3) {}, then the insertion point won't be before the first select, or is it handled in other way?
The insert point would be the branch or the first select *in the first block" (if any). This particular sentence is specifically about this example. If the selects are instead after if (c3) {}, there's no select in the first block and the insert point would be right before the branch.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:933
+  CHR_DEBUG(dbgs() << "findScopes " << R->getNameStr() << "\n");
+  CHRScope *Result = findScope(R);
+  // Visit subscopes.
----------------
davidxl wrote:
> Why can' this scope be merged with subsequent ones?
This will be merged with subsequent ones, if possible, after it's returned to the recursive call site in line 943. At the top level of the recursion, there's the sentinel top-level region at the root of the region tree, which doesn't have a subsequent region. So no merging opportunities missed.


Repository:
  rL LLVM

https://reviews.llvm.org/D50591