[PATCH] D50591: [PGO] Control Height Reduction

[David Li via Phabricator via llvm-commits]

davidxl 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);
----------------
is it possible to have S0 == S1?


================
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.
----------------
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?


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:930
+
+CHRScope * CHR::findScopes(Region *R, Region *NextRegion, Region *ParentRegion,
+                           SmallVectorImpl<CHRScope *> &Scopes) {
----------------
Add a description that this methods find all nested scopes and merge them if possible.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:933
+  CHR_DEBUG(dbgs() << "findScopes " << R->getNameStr() << "\n");
+  CHRScope *Result = findScope(R);
+  // Visit subscopes.
----------------
Why can' this scope be merged with subsequent ones?


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:935
+  // Visit subscopes.
+  if (R->begin() != R->end()) {
+    CHRScope *ConsecutiveSubscope = nullptr;
----------------
This early check does not seem necessary -- it does not save much compile time, but makes the code less clean.


Repository:
  rL LLVM

https://reviews.llvm.org/D50591