[PATCH] D60295: [CodeGen] Replace -max-jump-table-size with -max-jump-table-targets

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

I think this keeps the algorithm still withing O(n^2) so that's good.

But I am worried that this adds more complexity to something that is already very complex. Can you share numbers to show that this is worth it?

I left some comments for how to maybe make this cleaner, and I think that's the main improvement that's needed: it needs be easier to read the code, understand what's happening and see that it's correct.

I also worry that this makes the code slower, even though the new functionality is not used by most targets. Even though it's still O(n^2) time complexity, we're doing more work now. Can you measure and see how this affects compile time of a large switch? (Maybe try the program from https://bugs.llvm.org/show_bug.cgi?id=23490)



================
Comment at: llvm/lib/CodeGen/SwitchLoweringUtils.cpp:142
+  // targets.
+  struct PartitionTraits {
+    uint64_t Range;
----------------
Maybe PartitionStats is a better name, since it's really about numbers, not any other kinds of traits.


================
Comment at: llvm/lib/CodeGen/SwitchLoweringUtils.cpp:147
+  };
+  // PartitionTarget[x] is the traits for the partition Clusters[i..j],
+  // where x is j - i - 1.
----------------
I guess you mean PartitionTrait, not PartitionTarget.
Also, the indexing of this seems very confusing, and that seems to be because elements are inserted with push_back().

Couldn't PartitionTrait[x] be the traits for Clusters[i..x], i.e. a straight-forward indexing?

Also, since it's a vector, I don't think the name should be in singular.


================
Comment at: llvm/lib/CodeGen/SwitchLoweringUtils.cpp:166
+    for (int64_t j = i + 1; j < N; j++) {
+      Trait.Range = getJumpTableRange(Clusters, i, j);
+      Trait.Cases = getJumpTableNumCases(TotalCases, i, j);
----------------
Using the same variable, defined on line 158, both for initializing the array, and then for lookups later is confusing.

I think it would be easier to read of the code in this block was more like:

  PartitionTraits[j].Range = ...
  PartitionTraits[j].Cases = ...
  PartitionTraits[j].Targets = ...


================
Comment at: llvm/lib/CodeGen/SwitchLoweringUtils.cpp:179
       // Try building a partition from Clusters[i..j].
-      Range = getJumpTableRange(Clusters, i, j);
-      NumCases = getJumpTableNumCases(TotalCases, i, j);
-      assert(NumCases < UINT64_MAX / 100);
-      assert(Range >= NumCases);
+      Trait = PartitionTrait[j - i - 1];
 
----------------
Here the indexing gets confusing (also we probably don't want to copy the struct, but use a const-ref).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60295/new/

https://reviews.llvm.org/D60295