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

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

Thanks! I think that's looking better.

Sorry for the slow reply; I was travelling the last couple of days. Some comments inline.



================
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:
> How was the value 66 chosen?
I'm still curious about this.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:138
+static cl::opt<unsigned> DominantCasePercentThreshold(
+    "dominant-case-percent-threshold", cl::Hidden, cl::init(66),
+    cl::desc("Set the case probability threshold for peeling the case from a "
----------------
Perhaps the option name could make it more clear that this is about switch peeling. Maybe "-switch-peel-threshold"?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9839
 
+inline static BranchProbability
+scaleCaseProbality(BranchProbability CaseProb, BranchProbability SwitchProb) {
----------------
No need for 'inline'. Can you add a comment that explains what this does?


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


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9849
+MachineBasicBlock *
+SelectionDAGBuilder::peelOneDominantWorkItem(const SwitchInst &SI,
+                                             CaseClusterVector &Clusters,
----------------
Maybe call it peelDominantCaseCluster instead, since it's operating on the CaseClusters.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9865
+    if (CC.Low != CC.High)
+      continue;
+    if (CC.Prob <= TopCaseProb)
----------------
Why are you skipping clusters where Low != High?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9866
+      continue;
+    if (CC.Prob <= TopCaseProb)
+      continue;
----------------
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.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9880
+  ++BBI;
+  MachineBasicBlock *PeeledSwitchMBB =
+      FuncInfo.MF->CreateMachineBasicBlock(SwitchMBB->getBasicBlock());
----------------
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.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9883
+  FuncInfo.MF->insert(BBI, PeeledSwitchMBB);
+  auto PeeledCaseIt = Clusters.begin() + PeeledCaseIndex;
+
----------------
Maybe move this down to where it's used (when creating W).


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9917
+  MachineBasicBlock *PeeledSwitchMBB =
+      peelOneDominantWorkItem(SI, Clusters, PeeledProb);
+
----------------
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).


================
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() &&
----------------
When would DefaultMBB get replaced? If it can, it needs a test case.


================
Comment at: test/CodeGen/Generic/switch-lower-peel-top-case.ll:13
+  ], !prof !2
+; CHECK: Peeled one top case in switch stmt, prob: 0x5999999a / 0x80000000 = 70.00%
+; CHECK: Scale the probablity for one cluster, before scaling: 0x0020c49c / 0x80000000 = 0.10%
----------------
Checking the debug printouts doesn't seem very nice. Can't we check the resulting machine-IR and make sure it has the correct probabilities?


https://reviews.llvm.org/D39262