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

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

In https://reviews.llvm.org/D31085#737231, @junbuml wrote:

> > 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.
>
> For me, it seems that the code change in DAG and TLI only make sense when reviewed together with the changes in InlineCost.  That's why I put them together. If you generally agree with the change in DAG side code, I can break it as a separate patch, and leave only inliner side change in here.


Yes, reviewing them together makes total sense. I just wish the change we're making were smaller.

Anyway, I think this is really close now. Each time I read through it, it looks better :-)



================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:194
+
+    auto CI = SI.case_begin();
+    APInt MaxCaseVal = CI->getCaseValue()->getValue();
----------------
Declaring CI and starting to increment it outside the for-loop is a little unusual. I realize this is to avoid repeating the first iteration (maybe I wrote this somewhere?), but I think it would be better if this were written as a straight-forward loop:

```
APInt MaxCaseVal = SI.case_begin()->getCaseValue()->getValue();
APInt MinCaseVal = SI.case_begin()->getCaseValue()->getValue();
for (auto CI : SI.cases()) {
  ...
}
```


================
Comment at: include/llvm/Target/TargetLowering.h:771
+
+    if (NumClusters < 2 || NumClusters < getMinimumJumpTableEntries())
+      return false;
----------------
junbuml wrote:
> hans wrote:
> > 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.
> Yes, I agree that areJTsAllowed() should see only function and target, so I just pass a Function to areJTsAllowed(). But I don't think isSuitableForJumpTable is good place for this check because this check is for the whole clusters of a switch, but isSuitableForJumpTable should see a set of clusters, not necessary the whole clusters, especially when we try to build jump tables for split clusters in DAG. We also need to do an early exit when this check hit.  So I do this check in findJumpTables() in DAG and getEstimatedNumberOfCaseClusters() in TTI.
isSuitableForJumpTable() seems like a good place for this to me. It should be able to handle the whole switch range or a subset in the same way.


================
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;
----------------
junbuml wrote:
> hans wrote:
> > hans wrote:
> > > 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?
> > Oh never mind, you're capping the uint64_t at INT_MAX so it shuold work. Probably still don't want to do `Cost +=` though.
> Thanks for this. Yes, it should be Cost =, instead of Cost +=.
Cool, makes sense now. Are the `(uint64_t)` casts strictly necessary though? SwitchCost is already `uint64_t` so I'd imagine `INT_MAX` to get promoted and everything to work out?


================
Comment at: lib/Analysis/InlineCost.cpp:1018
+    // save compile-time.
+    int CostLowerBound = Cost + SI.getNumCases() * InlineConstants::InstrCost;
+    if (CostLowerBound > Threshold) {
----------------
Could this overflow?


https://reviews.llvm.org/D31085