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

[Hans Wennborg via Phabricator via llvm-commits]

hans added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:773
                                ArrayRef<const Value *> Arguments) = 0;
+  virtual int getEstimatedNumberOfCaseClusters(const SwitchInst &SI,
+                                               unsigned &JTSize) = 0;
----------------
junbuml wrote:
> hans wrote:
> > The other function here return an estimated cost for lowering. I think that would be a better interface for this too.
> I doubt if this is good place to get the inline cost because other functions handle  user costs which is different from inline cost. Mixing the user cost and inline cost here might be a bad  choice. I believe the inline cost should be decided in InlineCost. 
I was thinking it doesn't need to be used as the inlining cost directly, but exposing some metric the inline cost model could then make use of - that's kind of what we're doing anyway.

What I don't like about this one is that it feels a little out of place compared to other functions which return an estimate of the cost of lowering an instruction, whereas this seems to return details about the lowering instead.

I'm not sure how to make this better though.


================
Comment at: include/llvm/Target/TargetLowering.h:766
+  /// Return true if lowering to a jump table is allowed.
+  bool areJTsAllowed(const SwitchInst *SI, int64_t NumCluster) const {
+    const Function *Fn = SI->getParent()->getParent();
----------------
NumCluster -> NumClusters please


================
Comment at: lib/Analysis/InlineCost.cpp:1035
+
+    // Considering forming a BTree, we should find the number of nodes which is
+    // same as the number of comparisons when lowered. For a given number of
----------------
I'm not sure it's a BTree, but rather a regular binary search tree.


================
Comment at: lib/Analysis/InlineCost.cpp:1048
+      unsigned Exp = Log2_64_Ceil(NumCaseCluster) - 1;
+      uint64_t NumberOfNonLeaf = ((uint64_t) 1 << (Exp)) - 1;
+      uint64_t ExpectedNumberOfCompare = NumCaseCluster + NumberOfNonLeaf;
----------------
Very nice! I think the code can be simplified though, and this will allow us to avoid the rounding from Log2_64_Ceil: (N = NumCaseCluster)

`NumberOfNonLeaf = 2^(log2(N) - 1) - 1 = 2^log2(N) * 2^-1 - 1 = N/2 - 1`
Adding NumCaseCluster to that yields `N + N / 2 - 1 = N * 3 / 2 - 1`.




I kind of wish this is all there was to this change and we didn't need all that other code: quick check if the switch is suitable for a bit test or jump table, otherwise compute a cost based on `N * 3 / 2 - 1`. I don't know if it's possible though.


================
Comment at: lib/Analysis/InlineCost.cpp:1053
+      uint64_t SwitchCost = ExpectedNumberOfCompare * 2 * InlineConstants::InstrCost;
+      Cost += std::min((uint64_t)INT_MAX, SwitchCost + (uint64_t) Cost);
+    } else
----------------
Can we really run into INT_MAX here? Especially given the Threshold check above?


================
Comment at: lib/Analysis/InlineCost.cpp:1055
+    } else
+      Cost += NumCaseCluster * 2 * InlineConstants::InstrCost;
+
----------------
I would suggest handling the NumCaseCluster <= 3 first and returning early for that, since it's the less complicated case.


https://reviews.llvm.org/D31085