[PATCH] D31080: [DAG] Extract switch lowering as a spearate object NFC

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

In https://reviews.llvm.org/D31080#712466, @junbuml wrote:

> From Hans' comment, decouple cluster calculation from lowering. Introduced a new structure CaseCluster which is only for cluster calculation.


Thanks! Things are getting better. I've made some comments, but haven't looked carefully at the changes to SelectionDAGBuilder.h/cpp yet.

> MachineCaseCluster will be filled from CaseCluster before lowering.

This seems a little unfortunate, because it seems a MachineCaseCluster basically includes the same info as a CaseCluster with some more fields.

Would it be possible instead to do the lowering using just the CaseCluster objects, and maybe storing any auxiliary data on the side?

Before we get any further, I also would like to ask if you have done any measurements of compile-time with this set of patches. As I said before, I think this be quite an expensive hook to call for the inline cost analysis, and it would be nice to see some numbers. If it turns out that it is expensive, perhaps we could come up with some better inline cost heuristic, perhaps something based on the density of the switch.



================
Comment at: include/llvm/CodeGen/SwitchCaseCluster.h:41
+  const ConstantInt *Low, *High;
+  CaseVector Cases;
+
----------------
This should probably be a SmallVector since it will often contain only one element. I'm also not sure if having a typedef for it is really helpful. There needs to be a comment explaining that the vector holds case indexes from the switch.


================
Comment at: include/llvm/CodeGen/SwitchCaseCluster.h:69
+      assert(getCaseSuccessorAt(SI, I) == BB && "Invalid case cluster");
+#endif
+    return BB;
----------------
I'm not sure this assert is worth it.


================
Comment at: include/llvm/CodeGen/SwitchCaseCluster.h:89
+    C.High = High;
+    return C;
+  }
----------------
But how is C.Cases set?


================
Comment at: include/llvm/CodeGen/SwitchCaseCluster.h:104
+
+class TargetLowering;
+
----------------
Since TargetLowering.h is included, I guess this isn't needed.


================
Comment at: include/llvm/CodeGen/SwitchCaseCluster.h:131
+  /// Extract cases from the switch and build inital form of case clusters.
+  /// Return a default block.
+  const BasicBlock *formInitalCaseClusers(const SwitchInst &SI,
----------------
What does return a default block mean? The default basic block of the switch? Why return it?
Spelling, there's an i missing in initial.


================
Comment at: include/llvm/CodeGen/SwitchCaseCluster.h:142
+  // Replace an unreachable default with the most popular destination.
+  const BasicBlock *replaceUnrechableDefault(const SwitchInst &SI,
+                                             CaseClusterVector &Clusters);
----------------
Spelling: missing a in unreachable.


================
Comment at: include/llvm/CodeGen/SwitchCaseCluster.h:150
+
+  /// Return true if it can build a bit test cluster from Clusters[First..Last].
+  // bool canBuildJumpTable(const CaseClusterVector &Clusters, unsigned First,
----------------
What is "it" here?
The comment on the next line looks like leftover from earlier code?
Same thing on the next method.


================
Comment at: lib/CodeGen/SelectionDAG/CMakeLists.txt:26
   TargetLowering.cpp
-  
   DEPENDS
----------------
No need to remove this blank line, I think.


https://reviews.llvm.org/D31080