[PATCH] D50591: [PGO] Control Height Reduction

[Hiroshi Yamauchi via Phabricator via llvm-commits]

yamauchi added inline comments.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:745
+      CHR_DEBUG(dbgs() << "BI null\n");
+    if (BI && BI->isConditional()) {
+      BasicBlock *S0 = BI->getSuccessor(0);
----------------
davidxl wrote:
> Reads better if it is an early return 'return nullptr'
Can't do an early return here because we will fall through to look for selects below.


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:754
+        RegInfo RI(R);
+        RI.HasBranch = CheckBiasedBranch(
+            BI, R, TrueBiasedRegionsGlobal, FalseBiasedRegionsGlobal,
----------------
davidxl wrote:
> Should it return nullptr if HasBranch == false?
Likewise, we don't want to return yet because we look for selects below and because we try to curb out a larger scope (keep scopes connected) even if the branch is unbiased in the middle to increase the chance/scope of the branch merging (eg. merging three branches h1, u, h2 in a row into one where h1 and h2 are biased and u isn't).


================
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;
----------------
davidxl wrote:
> can depend on or 'can't' depend on.  It is unclear what the comment means. Perhaps explain more?
It means to say a branch instruction doesn't produce a value (in terms of SSA) so no other instruction can't have a use-def edge (data dependence) to a branch instruction. Updated comment. 


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:845
+                                    DT, Unhoistables, nullptr);
+      if (!IsHoistable) {
+        CHR_DEBUG(dbgs() << "Dropping select " << *SI << "\n");
----------------
davidxl wrote:
> this code is confusing:
> 
> if (not hoistable) {
>     unhoistable.erase(...);
> }
> 
Since we initialized Unhoistables with the selects above and we are dropping this select, we also remove it from Unhoistables. Added a comment. 


================
Comment at: lib/Transforms/Instrumentation/ControlHeightReduction.cpp:852
+    }
+    InsertPoint = getBranchInsertPoint(RI);
+    CHR_DEBUG(dbgs() << "InsertPoint " << *InsertPoint << "\n");
----------------
davidxl wrote:
> Line 827 does the same. Should it be moved above line 826?
This is not redundant. Since we may remove a select above, the insert point may change here. Added a comment.


Repository:
  rL LLVM

https://reviews.llvm.org/D50591