[PATCH] D50591: [PGO] Control Height Reduction

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:739
+  }
+  if (Exit && !loopy) {
+    auto *BI = dyn_cast<BranchInst>(Entry->getTerminator());
----------------
Add an comment describing the source pattern  for this case.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:739
+  }
+  if (Exit && !loopy) {
+    auto *BI = dyn_cast<BranchInst>(Entry->getTerminator());
----------------
davidxl wrote:
> Add an comment describing the source pattern  for this case.
perhaps  do

if (loopy)
  return nullptr;



================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:745
+      CHR_DEBUG(dbgs() << "BI null\n");
+    if (BI && BI->isConditional()) {
+      BasicBlock *S0 = BI->getSuccessor(0);
----------------
Reads better if it is an early return 'return nullptr'


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:754
+        RegInfo RI(R);
+        RI.HasBranch = CheckBiasedBranch(
+            BI, R, TrueBiasedRegionsGlobal, FalseBiasedRegionsGlobal,
----------------
Should it return nullptr if HasBranch == false?


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:764
+  }
+  if (!EntryInSubregion && !loopy) {
+    SmallVector<SelectInst *, 8> Selects;
----------------
describe the source pattern here.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:783
+    if (Selects.size() > 0) {
+      if (!Result) {
+        CHR_DEBUG(dbgs() << "Found a select-only region\n");
----------------
The code can be simplified:

auto AddSeelctsRegion = [&](RegionInfo &RegI) {
      for (auto *SI : Selects) {
          if (CheckBiasedSelect(SI, R,
                                TrueBiasedSelectsGlobal,
                                FalseBiasedSelectsGlobal,
                                SelectBiasMap))
            RI.Selects.push_back(SI);
        }
};

if (!Result) {
    RegInfo RI(R);
    AddSelectsRegion(&RI);
   Result = new CHRScope(RI);
   Scopes.insert(Result);
} else {
   AddSelectsRegion(Result->RegionInfos[0].R);
}



================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:814
+
+// Check that any of the branch and the selects in the region could be
+// hoisted above the the CHR branch insert point (the most dominating of
----------------
It is unclear from the comments what the method is check. Perhaps using a little C example to show what it checks.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:830
+    // Avoid a dependence from a select or a branch to a(nother)
+    // select. Note no instruction can't depend on a branch.
+    DenseSet<Instruction *> Unhoistables;
----------------
can depend on or 'can't' depend on.  It is unclear what the comment means. Perhaps explain more?


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:845
+                                    DT, Unhoistables, nullptr);
+      if (!IsHoistable) {
+        CHR_DEBUG(dbgs() << "Dropping select " << *SI << "\n");
----------------
this code is confusing:

if (not hoistable) {
    unhoistable.erase(...);
}



================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:852
+    }
+    InsertPoint = getBranchInsertPoint(RI);
+    CHR_DEBUG(dbgs() << "InsertPoint " << *InsertPoint << "\n");
----------------
Line 827 does the same. Should it be moved above line 826?


Repository:
  rL LLVM

https://reviews.llvm.org/D50591