[PATCH] D18223: Codegen: Decrease minimum jump table density

[Hans Wennborg via llvm-commits]

hans added a comment.

Thanks! I think this basically looks good.

Now that you're passing flags to llc instead, do we have any tests checking that the "optsize" and "minsize" function attributes have the desired effect?

We should probably have a test with functions with no attribute, optsize, and minsize that verifies the thresholds.

And as Eric said, some numbers showing how binary size (e.g. a self-hosted clang build) and perf are affected would be great.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:89
@@ -88,1 +88,3 @@
 
+/// Minimum jump table density for normal
+static cl::opt<unsigned>
----------------
for normal what?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:91
@@ +90,3 @@
+static cl::opt<unsigned>
+SparseJumpTableDensity("sparse-jump-table-density", cl::init(10), cl::Hidden,
+                       cl::desc("Minimum density for building a jump table in "
----------------
For a user who wants to tweak these flags, I'm not sure if the "sparse-" and "dense-" names are the most friendly.

What would you think of calling them "-jump-table-density" and "-optsize-jump-table-density"?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:95
@@ +94,3 @@
+
+/// Minimum jump table density for -Os or -Oz functions
+static cl::opt<unsigned>
----------------
ultra nit: period at end of comment.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8042
@@ +8041,3 @@
+  if (DefaultMBB->getParent()->getFunction()->optForSize())
+    Density = DenseJumpTableDensity;
+  if (N >= MinJumpTableSize
----------------
Dense jump table density is dense? :-)
I think this variable name would come out better if the flag was renamed as suggested above.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:308
@@ -310,3 +307,3 @@
   bool isDense(const CaseClusterVector &Clusters, unsigned *TotalCases,
-               unsigned First, unsigned Last);
+               unsigned First, unsigned Last, unsigned Density);
 
----------------
How about calling the new parameter MinDensity, RequiredDensity, or something like that to indicate that it's a threshold that the actual density gets compared against?


http://reviews.llvm.org/D18223