[PATCH] D60295: [CodeGen] Replace -max-jump-table-size with -max-jump-table-targets
Hans Wennborg via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 9 07:20:47 PDT 2019
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
More information about the llvm-commits
mailing list