[PATCH] D25212: Moderate the number of jump tables

[Hans Wennborg via llvm-commits]

hans added a comment.

Thanks! I think the functionality is really good, and this is almost ready to go in. I only have a few stylistic nits left.

Maybe the patch title could be more descriptive, because we're not just reducing the number of jump tables here, we're specifically making the code smarter about how to break ties when partitioning a switch in search for jump tables. So maybe: "Switch lowering: break ties smarter when partitioning switch for jump tables"



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8482
   SmallVector<unsigned, 8> LastElement(N);
-  // NumTables[i]: nbr of >= MinJumpTableSize partitions from Clusters[i..N-1].
-  SmallVector<unsigned, 8> NumTables(N);
+  // PartitionScore[i] is used to break ties when choosing between two
+  // partitionings resulting in the same number of partitions.
----------------
Since this isn't just the score of one partition, but all partitions starting with the i'th cluster, the name should reflect that, e.g. PartitionsScore or PartitioningScore.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8485
+  SmallVector<unsigned, 8> PartitionScore(N);
+  // Scores relative to generating a jump table to score partitions.
+  // Here, a small number of comparisons is considered as good as a jump table
----------------
The wording here doesn't seem very clear. What does "Score relative to generating a jump table to score partitions." mean?

We've already commented on how this works for the PartitionsScore vector above, so maybe we can just refer to that:

"For PartitionsScore, a small number of comparisons is considered as good as a jump table and ... ".


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8524
+        unsigned Score = j == N - 1 ? 0 : PartitionScore[j + 1];
+        if (IsTable)
+          Score += PartitionScore::Table;
----------------
I'd suggest checking IsOne first, then IsSmall and IsTable last to avoid the "IsSmall && !IsOne" situation below.

Also, I'm not sure how much the bool variables buy us here; it might be just as clear to just put the conditions in the if statements directly. Up to you.


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:48
 static cl::opt<unsigned> MaximumJumpTableSize
-  ("max-jump-table", cl::init(0), cl::Hidden,
-   cl::desc("Set maximum number of jump table entries; zero for no limit."));
+  ("max-jump-table-size", cl::init(0), cl::Hidden,
+   cl::desc("Set maximum size of jump table; zero for no limit."));
----------------
This seems unrelated so should be done in another patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D25212