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

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

Apologies for the reviews dragging out.

I think we've got the right idea with computing inline cost based on an estimate of the number of nodes in binary search tree, but the patch is still making a lot of changes all over the place making it hard to review.

The smaller you can make this change, the easier it will be to get this committed.



================
Comment at: include/llvm/Target/TargetLowering.h:771
+
+    if (NumClusters < 2 || NumClusters < getMinimumJumpTableEntries())
+      return false;
----------------
This check should probably be done in `isSuitableForJumpTable`. There should be no need to pass NumClusters to `areJTsAllowed` since whether jump tables are allowed only depends on the target and function. The switch instruction also shouldn't be passed in, just the function.


================
Comment at: include/llvm/Target/TargetLowering.h:779
+  /// Check whether the range [Low,High] fits in a machine word.
+  bool rangeFitsInWord(const APInt &Low, const APInt &High,
+                       const DataLayout &DL) const {
----------------
This is used in isSuitableForBitTests, but does it really need to be exposed in the TargetLowering interface?


================
Comment at: include/llvm/Target/TargetLowering.h:792
+  bool isSuitableForJumpTable(const SwitchInst *SI, unsigned JumpTableSize,
+                              uint64_t NumCases, uint64_t Range) const {
+    const bool OptForSize = SI->getParent()->getParent()->optForSize();
----------------
What's the difference between Range and JumpTableSize? As far as I can tell, those are always the same.


================
Comment at: lib/Analysis/InlineCost.cpp:1057
+    uint64_t SwitchCost = ExpectedNumberOfCompare * 2 * InlineConstants::InstrCost;
+    Cost += std::min((uint64_t)INT_MAX, SwitchCost + (uint64_t) Cost);
+    return false;
----------------
Aren't you adding `Cost` twice here? You're doing `Cost +=` and also `SwitchCost + Cost`.

Oh, Cost is just a regular int; yeah then I see how it can overflow. But std::min is returnning an uint64_t here, so it seems you're still not handling the overflow?


https://reviews.llvm.org/D31085