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

[Rong Xu via Phabricator via llvm-commits]

xur added inline comments.


================
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:
> xur wrote:
> > 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.
> I still don't understand why it matters here if DefaultMBB has changed?
When the DefaultMBB is changed (replaced with the most popular case), the branch probability not longer consistent.  If I scale the original default case branch probability, it could create a insane probability (greater than 100%).


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9881
+
+  // Record the MBB for the peeled switch statement. If there is only case
+  // is peeled. This points to the MBB of the default case.
----------------
hans wrote:
> nit: "If there is only case is peeled." sentence looks weird.
No need with the new patch.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9890
+    FuncInfo.MF->insert(BBI, PeeledSwitchMBB);
+  } else
+    PeeledSwitchMBB = FuncInfo.MBBMap[SI.getDefaultDest()];
----------------
hans wrote:
> Actually, if there's only one cluster, maybe we shouldn't be peeling. I mean, it doesn't make much sense right? And maybe that would help downstream code that could then assume that there are still cases left after peeling.
Ah. Right. Current logic handles it well. I will have a check that bailout earlier.


================
Comment at: test/CodeGen/Generic/switch-lower-peel-top-case.ll:12
+    i32 129, label %sw.bb7
+  ], !prof !2
+
----------------
hans wrote:
> This could probably be simplified a bit. If %n were an i32 instad of i16 you don't need the sext. You could drop local_unnamed_addr.
> 
> The case labels look pretty random; maybe chose cases with more purpose, and rename the target blocks to match. It would also be good to test the case where two adjacent cases are merged into a cluster and peeled.
This is extracted from a real program. But I agree that it should be simplified by removing the unrelated details. I will change. 


================
Comment at: test/CodeGen/Generic/switch-lower-peel-top-case.ll:12
+    i32 129, label %sw.bb7
+  ], !prof !2
+
----------------
hans wrote:
> xur wrote:
> > hans wrote:
> > > This could probably be simplified a bit. If %n were an i32 instad of i16 you don't need the sext. You could drop local_unnamed_addr.
> > > 
> > > The case labels look pretty random; maybe chose cases with more purpose, and rename the target blocks to match. It would also be good to test the case where two adjacent cases are merged into a cluster and peeled.
> > This is extracted from a real program. But I agree that it should be simplified by removing the unrelated details. I will change. 
> The part about case values, and test for peeling two adjacent cases still applies.
Sorry I might misread your earlier comments. Do you mean I should change the case value?


https://reviews.llvm.org/D39262