[PATCH] D25212: Moderate the number of jump tables

[Evandro Menezes via llvm-commits]

evandro marked 2 inline comments as done.
evandro 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;
 
----------------
hans wrote:
> Is deleting this function body intentional? Dito for the "set" method below.
Yes, since I moved them to `lib/CodeGen/TargetLoweringBase.cpp` as I created the `cl::opt` `min-jump-table-entries` to allow easy experimentation with this value.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8550
+             ((MoreJumpTables && Tables > NumTables[i]) ||
+              (!MoreJumpTables && Tables < NumTables[i])))) {
           MinPartitions[i] = NumPartitions;
----------------
hans wrote:
> evandro wrote:
> > hans wrote:
> > > evandro wrote:
> > > > hans wrote:
> > > > > I really don't like this. The purpose of this algorithm is to find jump tables. If you don't want to find jump tables, don't run it. If you think it's generating too large or sparse jump tables, tweak those parameters.
> > > > I'd like to have the option to choose more of fewer jump tables when the number of partitions is the same.  It's not clear to me why only one possibility was cast in the code originally, but it doesn't seem to me to be always the best choice.
> > > >  It's not clear to me why only one possibility was cast in the code originally, but it doesn't seem to me to be always the best choice.
> > > 
> > > It's because the whole purpose of the code is to find jump tables.
> > > 
> > > Consider the @optimal_jump_table2 test in test/CodeGen/X86/switch.ll. The cases in the switch can be partitioned as {0,1,2,9},{14,15} or {0,1,2},{9,14,15}. The code ensures that the partitioning with the jump table is chosen.
> > > 
> > > If you think that's the wrong lowering for your target, maybe increasing the minimum jump table entries is what you want.
> > Consider that function `jt1` in `test/CodeGen/AArch64/max-jump-table.ll` ends up with partitions {0..7}, {8..12},{13..16} with `-max-jump-table=8`, whereas arguably having its jump table partitioned as {0..7},{8..15} and an extra link in the `if-then-else` chain would be better performing.
> > 
> > I find the hard wired choice of always favoring more jump tables arbitrary.  The algorithm works rather well as is if favoring fewer jump tables.
> It's not arbitrary; it makes the algorithm work, as shown by the example in my previous comment.
> 
> Your example is interesting, though. I guess the question is, how do we teach the algorithm that  {0..7},{8..15},{16} is better than {0..7},{8..12},{13..16}?
> 
> The "pick the partitioning with more jump tables" rule is there to break ties in situations like my previous example:  {0,1,2,9},{14,15} vs {0,1,2},{9,14,15}.
> 
> However, a partition with a single case (cluster), {16} in your example is clearly as good as a jump table -- in fact it's better.
> 
> To model this, instead of having NumTables[i], we could have a PartitioningScore[i]. In addition to scoring 1 when the partition has >= MinJumpTableEntries, we could score 2 for a single-element partition (and perhaps also 1 for a two-element partition).
> 
> What do you think?
I really like your idea.  I'll try to translate it into code and run some tests before updating the diff.

Watch this space.


================
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);
----------------
hans wrote:
> 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.
Accumulating the scores seemed to distort the relative merits of the partitions.  I got more consistent results by isolating the score to just the individual partition.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8490
   LastElement[N - 1] = N - 1;
-  NumTables[N - 1] = 0;
+  PartitionScore[N - 1] = 0;
 
----------------
hans wrote:
> 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.
Right.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8511
+        bool IsTable = NumEntries >= MinJumpTableEntries;
+        bool IsSmall = NumEntries <= SmallJumpTableEntries;
+        bool IsOnlyOne = NumEntries == 1;
----------------
hans wrote:
> 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.)
It seemed to me that, if a target maintainer changes `min-jump-table-entries`, it's an important metric.  It made sense to use a threshold that'd be related to this metric.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8513
+        bool IsOnlyOne = NumEntries == 1;
+        unsigned Score = IsTable + IsSmall + IsOnlyOne;
+
----------------
hans wrote:
> 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]);
I'll spell out in a comment what is intended.

But I'm still debating with myself if the scoring should be cumulative or not.


Repository:
  rL LLVM

https://reviews.llvm.org/D25212