[PATCH] D25212: Moderate the number of jump tables

[Hans Wennborg via llvm-commits]

hans added a comment.

Apologies for the slow reply.

I think this is getting close.. some comments inline.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8485
+  SmallVector<unsigned, 8> PartitionScore(N);
+  // Weights relative to generating a jump table to score partitions.
+  // Here, a small number of comparisons is considered as good as a jump table
----------------
I would call these scores consistently instead of weights.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8488
+  // and a single comparison is considered better than a jump table.
+  enum PartitionScoreEnum : unsigned {
+    None = 0,
----------------
I'd drop "Enum" from the name, just "PartitionScore" sounds like a good name to me.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8489
+  enum PartitionScoreEnum : unsigned {
+    None = 0,
+    Table = 1,
----------------
Hmm, "None" doesn't seem like a great name. Maybe "Default" is better?

This would be used for partitions that are too small to be a table, but not small enough to get a better score. I'm not sure what to name that though..

How about something like:

NoTable = 0
FewCases = 1
Table = 2
SingleCase = 2


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8498
   LastElement[N - 1] = N - 1;
-  NumTables[N - 1] = 0;
+  PartitionScore[N - 1] = PartitionScoreEnum::Table;
 
----------------
Since this is a one-element partition, shouldn't it be PartitionScoreEnum::One?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8506
     LastElement[i] = i;
-    NumTables[i] = NumTables[i + 1];
+    PartitionScore[i] = PartitionScore[i + 1] + 1;
 
----------------
I think this should be  "+ PartitionScoreEnum::One" instead of "+ 1" since it's the score for a single-element partition we want to add.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8509
     // Search for a solution that results in fewer partitions.
-    for (int64_t j = N - 1; j > i; j--) {
+    for (int64_t j = N - 1; j >= i; j--) {
       // Try building a partition from Clusters[i..j].
----------------
Why change > to >= here?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8524
+        unsigned Score = j == N - 1
+                         ? PartitionScoreEnum::None : PartitionScore[j + 1];
+        if (IsTable)
----------------
I think it should be 0 instead of PartitionScoreEnum::None here.

The "j == N - 1" is used to check whether the current partition is the last one, which means there is no score to add from later partitions.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8528
+        else if (IsSmall || IsOne)
+          Score += IsOne ? PartitionScoreEnum::One : PartitionScoreEnum::Small;
 
----------------
It would be clearer to just have another else if-bransh instead of combining two cases into one here.


Repository:
  rL LLVM

https://reviews.llvm.org/D25212