[PATCH] Switch lowering: extract jump tables and bit tests before building binary tree (PR22262)
Daniel Jasper
djasper at google.com
Mon Mar 30 06:39:25 PDT 2015
>From a first look, this looks fantastic and is exactly what I had planned. Added a first round of comments, but haven't fully reviewed everything yet.
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2104
@@ +2103,3 @@
+ [](const CaseCluster &a, const CaseCluster &b) {
+ assert(a.Low == a.High && b.Low == b.High);
+ return a.Low->getValue().slt(b.Low->getValue());
----------------
This assert could use a comment. I presume it checks whether all of the input Clusters contain exactly one case?
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2112
@@ +2111,3 @@
+ const unsigned N = Clusters.size();
+ while (SrcIndex < N) {
+ CaseCluster &CC = Clusters[SrcIndex];
----------------
Why isn't this a for loop?
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2134
@@ -2688,2 +2133,3 @@
+
void SelectionDAGBuilder::UpdateSplitBlock(MachineBasicBlock *First,
----------------
Don't add empty line.
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7129
@@ +7128,3 @@
+ unsigned Last) {
+ static const int MinDensity = 40;
+
----------------
I find a bit odd to use percentages here and accept the resulting rounding error. Even using 4 and 10 would lead to better accuracy. Not at all important, I just don't understand it ;-).
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7165
@@ +7164,3 @@
+#ifndef NDEBUG
+ // Clusters must be non-empty, sorted, and only contain Range clusters.
+ assert(!Clusters.empty());
----------------
Wouldn't it be even easier to build jump tables before rangifying the cases? Seems like that would make a lot of the specific TotalCases logic unnecessary. Then, you could rangify everything that is left after building jump tables?
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7178
@@ +7177,3 @@
+ // Minimum nbr of clusters in a jump table.
+ static const int MinJTSize = 4;
+
----------------
Is this the same behavior as before? If I understand correctly, you now wouldn't turn:
case 1:
case 2:
case 3: return 1;
case 4: return 2;
into a jump table as these are only two clusters. I think before, it might have been counted as 4 values with a density of 50% so it would have been turned into jump table.
Also: The minimum size should be controlled by TLI.getMinimumJumpTableEntries()!
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:138
@@ +137,3 @@
+ enum CaseClusterKind {
+ /// A cluster of adjacent case labels with the same destination.
+ CC_Range,
----------------
Maybe extend the comment to explain that this is also used for trivial 1-element ranges.
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:147
@@ +146,3 @@
+ /// A cluster of case labels.
+ struct CaseCluster {
+ CaseClusterKind Kind;
----------------
I'd probably make this a proper class and make the fields const to show that they aren't mutable.
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:211
@@ +210,3 @@
+
+ /// Merge adjacent cases and sort Clusters.
+ void Clusterify(CaseClusterVector &Clusters);
----------------
The name is a bit confusing now. The input are already (trivial) clusters. SortAndRangify maybe?
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:303
@@ +302,3 @@
+ /// Find clusters of cases suitable for jump table lowering.
+ void FindJumpTables(CaseClusterVector &Clusters, const SwitchInst *SI,
+ MachineBasicBlock *DefaultMBB);
----------------
Functions should start lower-case. Here and everywhere else.
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:850-851
@@ -831,2 +849,4 @@
MachineBasicBlock *NextBlock(MachineBasicBlock *MBB);
+
+
};
----------------
Remove.
http://reviews.llvm.org/D8649
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list