[PATCH] Switch lowering: extract jump tables and bit tests before building binary tree (PR22262)

[Daniel Jasper]

>From a first look, this looks fantastic and is exactly what I had planned. Added a first round of comments, but haven't fully reviewed everything yet.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2104
@@ +2103,3 @@
+            [](const CaseCluster &a, const CaseCluster &b) {
+    assert(a.Low == a.High && b.Low == b.High);
+    return a.Low->getValue().slt(b.Low->getValue());
----------------
This assert could use a comment. I presume it checks whether all of the input Clusters contain exactly one case?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2112
@@ +2111,3 @@
+  const unsigned N = Clusters.size();
+  while (SrcIndex < N) {
+    CaseCluster &CC = Clusters[SrcIndex];
----------------
Why isn't this a for loop?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2134
@@ -2688,2 +2133,3 @@
 
+
 void SelectionDAGBuilder::UpdateSplitBlock(MachineBasicBlock *First,
----------------
Don't add empty line.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7129
@@ +7128,3 @@
+                                  unsigned Last) {
+  static const int MinDensity = 40;
+
----------------
I find a bit odd to use percentages here and accept the resulting rounding error. Even using 4 and 10 would lead to better accuracy. Not at all important, I just don't understand it ;-).

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7165
@@ +7164,3 @@
+#ifndef NDEBUG
+  // Clusters must be non-empty, sorted, and only contain Range clusters.
+  assert(!Clusters.empty());
----------------
Wouldn't it be even easier to build jump tables before rangifying the cases? Seems like that would make a lot of the specific TotalCases logic unnecessary. Then, you could rangify everything that is left after building jump tables?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7178
@@ +7177,3 @@
+  // Minimum nbr of clusters in a jump table.
+  static const int MinJTSize = 4;
+
----------------
Is this the same behavior as before? If I understand correctly, you now wouldn't turn:

case 1:
case 2:
case 3: return 1;
case 4: return 2;

into a jump table as these are only two clusters. I think before, it might have been counted as 4 values with a density of 50% so it would have been turned into jump table.


Also: The minimum size should be controlled by TLI.getMinimumJumpTableEntries()!

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:138
@@ +137,3 @@
+  enum CaseClusterKind {
+    /// A cluster of adjacent case labels with the same destination.
+    CC_Range,
----------------
Maybe extend the comment to explain that this is also used for trivial 1-element ranges.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:147
@@ +146,3 @@
+  /// A cluster of case labels.
+  struct CaseCluster {
+    CaseClusterKind Kind;
----------------
I'd probably make this a proper class and make the fields const to show that they aren't mutable.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:211
@@ +210,3 @@
+
+  /// Merge adjacent cases and sort Clusters.
+  void Clusterify(CaseClusterVector &Clusters);
----------------
The name is a bit confusing now. The input are already (trivial) clusters. SortAndRangify maybe?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:303
@@ +302,3 @@
+  /// Find clusters of cases suitable for jump table lowering.
+  void FindJumpTables(CaseClusterVector &Clusters, const SwitchInst *SI,
+                      MachineBasicBlock *DefaultMBB);
----------------
Functions should start lower-case. Here and everywhere else.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:850-851
@@ -831,2 +849,4 @@
   MachineBasicBlock *NextBlock(MachineBasicBlock *MBB);
+
+
 };
----------------
Remove.

http://reviews.llvm.org/D8649

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/