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

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

Starting to look really good.



================
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.
----------------
nit: "If there is only case is peeled." sentence looks weird.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9890
+    FuncInfo.MF->insert(BBI, PeeledSwitchMBB);
+  } else
+    PeeledSwitchMBB = FuncInfo.MBBMap[SI.getDefaultDest()];
----------------
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.


================
Comment at: test/CodeGen/Generic/switch-lower-peel-top-case.ll:12
+    i32 129, label %sw.bb7
+  ], !prof !2
+
----------------
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.


https://reviews.llvm.org/D39262