[PATCH] D21940: Add support to optionally limit the size of jump tables

[Evandro Menezes via llvm-commits]

evandro marked 6 inline comments as done.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8342
@@ -8337,1 +8341,3 @@
+    TotalCases[i] = (Clusters[i].High->getValue() -
+                     Clusters[i].Low->getValue()).getLimitedValue() + 1;
     if (i != 0)
----------------
hans wrote:
> No need to change the contents of this for-loop.
Like `JumpTableSize`, can't this overflow too?

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8352
@@ +8351,3 @@
+  unsigned JumpTableSize = (Clusters[N - 1].High->getValue() -
+                            Clusters[0].Low->getValue()).getLimitedValue() + 1;
+  if (JumpTableSize <= MaxJumpTableSize &&
----------------
hans wrote:
> What do you think about introducing a helper function that we could call like 'jumpTableSize(Clusters, 0, N-1)'? I think that would make this a little easier to read here and below, and in the function we could take care not to let the value overflow for example.
The operation seemed too short for a function, but saturating its return might make sense wrapping this in a function.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8408
@@ -8397,2 +8407,3 @@
         if (NumPartitions < MinPartitions[i] ||
-            (NumPartitions == MinPartitions[i] && Tables > NumTables[i])) {
+            (MaxJumpTableSize == UINT_MAX &&
+             NumPartitions == MinPartitions[i] && Tables > NumTables[i])) {
----------------
hans wrote:
> Why do we need this extra check? We're already guaranteed by the above that there won't be any tables greater than the max size.
This extra check is for backwards compatibility.  Though the table is no larger than `MaxJumpTableSize`, we may end up with two tables rather than one.  It might make sense to increase tables slightly to decrease the length of the `if-then-else` chain at the beginning, but when the size of the tables is limited, we might end up with a longer `if-then-else` chain, because tables smaller than `MinJumpTableEntries` would be more likely.  As a matter of fact, except to improve code size, it'd be better to create fewer jump tables, if it adds links to the `if-then--else` chain, as most processors can predict the outcome of each link better than of an indirect branch.  Without this condition, `max-jump-table.ll` below fails at the `jt1` function and the resulting code is 3 jump tables with a 3 comparisons instead of 2 jump tables with 3 comparisons.

================
Comment at: llvm/test/CodeGen/Generic/max-jump-table.ll:1
@@ +1,2 @@
+; RUN: llc %s -O2 -print-machineinstrs -jump-table-density=40                   -o - 2>%t; FileCheck %s --check-prefix=CHECK --check-prefix=CHECK0 <%t
+; RUN: llc %s -O2 -print-machineinstrs -jump-table-density=40 -max-jump-table=4 -o - 2>%t; FileCheck %s --check-prefix=CHECK --check-prefix=CHECK4 <%t
----------------
hans wrote:
> Running llc without a target triple might end up running for a target that doesn't do jump tables for switches.
> 
> I think this test needs to be under a target that we know uses jump tables. If you target ARM you can also test that ExynosM1 does get its desired max jump table size.
True that.



Repository:
  rL LLVM

https://reviews.llvm.org/D21940