[PATCH] D39262: [CodeGen] Peel off the dominant case in switch statement in lowering

[Rong Xu via Phabricator via llvm-commits]

xur marked 7 inline comments as done.
xur added a comment.

Hans, Thanks for the detailed reviews. Please check if the updated patch addresses your reviews.

One change not in the reviews is I move the code after rangeify clusters.  This is together with the suggested change that removing Low != High check.

I also change the code a bit so that if the single case is removed, generate BR to the default directly.



================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:141
+static cl::opt<unsigned> DominantCasePercentThreshold(
+    "dominant-case-percent-threshold", cl::Hidden, cl::init(66),
+    cl::desc("Set the case probability threshold (<=100) for peeling out from "
----------------
hans wrote:
> hans wrote:
> > How was the value 66 chosen?
> I'm still curious about this.
We have a case of ~70% that we would like to peel in our internal test. I lower a few percent as the buffering. 


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9847
+
+// Peel the top probability case if it exceeds the threshold.
+MachineBasicBlock *
----------------
hans wrote:
> Please comment on what the return value is, that is updates Clusters, and what the PelledProb value is.
Also changed the parameter for better clarity.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9865
+    if (CC.Low != CC.High)
+      continue;
+    if (CC.Prob <= TopCaseProb)
----------------
hans wrote:
> Why are you skipping clusters where Low != High?
I was concerning that this would create two jumps for the range. But
this turns out not to be the case. I moved this condition. 


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9866
+      continue;
+    if (CC.Prob <= TopCaseProb)
+      continue;
----------------
hans wrote:
> This requires CC.Prob so be strictly greater than the DominantCasePercentThreshold, not just greater or equal. Is that intentional? I'd assume that if the threshold is 80% probability, and we have a case with 80% probability, we want to peel it.
you are right! changed it to "<"


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9880
+  ++BBI;
+  MachineBasicBlock *PeeledSwitchMBB =
+      FuncInfo.MF->CreateMachineBasicBlock(SwitchMBB->getBasicBlock());
----------------
hans wrote:
> Is PeeledSwitchMBB the MBB that contains the peeled switch (i.e. the switch minus the most dominant case), or the one containing the peeled case? I wonder if we could find better names, or add comments to make this clear.
Containing the peeled switch statement. I added a comment here.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9917
+  MachineBasicBlock *PeeledSwitchMBB =
+      peelOneDominantWorkItem(SI, Clusters, PeeledProb);
+
----------------
hans wrote:
> Could we just update SwitchMBB instead, if that's the one that has the rest of the switch? (I'm a little confused by what SwitchMBB actually is at this point).
My first try was to update SwitchMBB directly, but it did not work.  It seems that we need SwitchMBB as the placehold to decide if we defer the case handling (i.e visitSwitchCase() or push_back to SwitchCases.)


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9994
+  // Scale the branchprobability for DefaultMBB if the peel occurs and
+  // DefaultMBB is not replaced.
+  if (PeeledProb != BranchProbability::getOne() &&
----------------
hans wrote:
> When would DefaultMBB get replaced? If it can, it needs a test case.
We replace the unreachable default with most popular destination.  This actually exposed by an existing test case.


https://reviews.llvm.org/D39262