[PATCH] D31085: [InlineCost] Increase the cost of Switch

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

It's starting to look much simpler, which is great.



================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:773
                                ArrayRef<const Value *> Arguments) = 0;
+  virtual int getEstimatedNumberOfCaseClusters(const SwitchInst &SI,
+                                               unsigned &JTSize) = 0;
----------------
The other function here return an estimated cost for lowering. I think that would be a better interface for this too.


================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:175
+  int getEstimatedNumberOfCaseClusters(const SwitchInst &SI,
+                                       unsigned &JumpTableSize) {
+    /// Try to find the estimated number of clusters. Note that the number of
----------------
I wish this could be much simpler. Maybe most of the code could defer to TLI::isSuitableForBitTest / isSuitableForJumpTable which could also be used from the DAG code.


================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:225
+
+    // Check if suitable for a jump table.
+    if (IsJTAllowed) {
----------------
If we have TLI::isSuitableForBitTests, maybe we should have isSuitableForJumpTable too, that way we don't have to duplicate as much logic, and that one could do what areJTsAllowed() as well.


================
Comment at: lib/Analysis/InlineCost.cpp:1049
+  if (JumpTableSize)
+    Cost += JumpTableSize * InlineConstants::InstrCost;
+
----------------
If the whole switch is suitable for a jump table, can't we just return after this?


================
Comment at: lib/Analysis/InlineCost.cpp:1051
+
+  while (!SwitchWorkList.empty()) {
+    unsigned NumCases = SwitchWorkList.back();
----------------
Why do we have to do this?

The code is basically constructing the tree and throwing it away. It should be possible to compute an estimate for the size of the tree with a closed-form mathematical expression.


https://reviews.llvm.org/D31085