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

[Hans Wennborg]

================
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());
----------------
djasper wrote:
> This assert could use a comment. I presume it checks whether all of the input Clusters contain exactly one case?
Right. Done.

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

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

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7129
@@ +7128,3 @@
+                                  unsigned Last) {
+  static const int MinDensity = 40;
+
----------------
djasper wrote:
> 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 ;-).
What rounding error do you mean?

It's no big deal, I just find "40%" easier to think about than "4 tenths".

================
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());
----------------
djasper wrote:
> 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?
The only reason (besides that the old code did it this way) is to reduce the amount of work in FindJumpTables, which if O(n^2) in the number of clusters. That might not make a big difference in practice, though..

Initially, I was worried about getting hit with case ranges like this: "case 0 ... 100000", but it turns out Clang won't lower those to switch cases anyway.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7178
@@ +7177,3 @@
+  // Minimum nbr of clusters in a jump table.
+  static const int MinJTSize = 4;
+
----------------
djasper wrote:
> 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()!
Excellent question, this is a little sketchy.

With the old code, your example would not lowered with a jump table, because handleSmallSwitchRange() would get to it first.

I think counting clusters is the right metric for deciding to build a table (in terms of size -- for density we count actual cases). A range is as cheap to lower as a single case in terms of branches, and it's the number of branches that matters here. It's possible JT-threshold should be 3 instead of 4, though.

Good point about TLI.getMinimumJumpTableEntries(), I'll fix that.

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

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:147
@@ +146,3 @@
+  /// A cluster of case labels.
+  struct CaseCluster {
+    CaseClusterKind Kind;
----------------
djasper wrote:
> I'd probably make this a proper class and make the fields const to show that they aren't mutable.
I do mutate them though, e.g. in Clusterify()  :-)
I can try to avoid that if you think it's bad.

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

================
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);
----------------
djasper wrote:
> Functions should start lower-case. Here and everywhere else.
Done.

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

http://reviews.llvm.org/D8649

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