[PATCH] D25212: Moderate the number of jump tables

[Hans Wennborg via llvm-commits]

hans added inline comments.


================
Comment at: llvm/include/llvm/Target/TargetLowering.h:1030
   /// Return lower limit for number of blocks in a jump table.
-  unsigned getMinimumJumpTableEntries() const {
-    return MinimumJumpTableEntries;
-  }
+  unsigned getMinimumJumpTableEntries() const;
 
----------------
Is deleting this function body intentional? Dito for the "set" method below.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8484
+  // by adding a small number of links in the if-then-else chain, especially if
+  // only one link is added
+  SmallVector<unsigned, 8> PartitionScore(N);
----------------
The comment needs to say what the vector index means.

How about: "ParitionsScore[i]: Score of partitions for Clusters[i..N-1]. The score is used to break ties when choosing between two partitionings resulting in the same number of partitions."

Note that this needs to be the sum of the scores for the individual partitions. I've commented about this below.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8490
   LastElement[N - 1] = N - 1;
-  NumTables[N - 1] = 0;
+  PartitionScore[N - 1] = 0;
 
----------------
This base case means putting Clusters[N-1] into a partition of its own, so the score should be that of a one-element partition.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8498
     LastElement[i] = i;
-    NumTables[i] = NumTables[i + 1];
+    PartitionScore[i] = PartitionScore[i + 1];
 
----------------
Same here: this case means putting Clusters[i] into a partition of its own. The total score should be that of a one-element partition plus PartitionScore[i + 1].


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8511
+        bool IsTable = NumEntries >= MinJumpTableEntries;
+        bool IsSmall = NumEntries <= SmallJumpTableEntries;
+        bool IsOnlyOne = NumEntries == 1;
----------------
Hmm, I wonder if we should start simple and just check for the jump table and one-element cases. (That name isn't great either -- SmallJumpTableEntries sounds like it's the number of elements in a small jump table, but that's not the case.)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8513
+        bool IsOnlyOne = NumEntries == 1;
+        unsigned Score = IsTable + IsSmall + IsOnlyOne;
+
----------------
Hmm, the computation of this score is subtle. A one-element partition gets a higher score than a jump table, but only because it's also <= SmallJumpTableEntries.

I'd prefer to spell things out here with an if-else chain and symbolic constants for the scores instead.

Also, we need the score for the whole partitioning, not just this partition, so I think this needs to be something like Score = ThisScore + (j == N - 1 ? 0 : PartitionScore[j + 1]);


Repository:
  rL LLVM

https://reviews.llvm.org/D25212