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

Jun Bum Lim via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Apr 25 12:26:17 PDT 2017


junbuml added a comment.

> 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.



================
Comment at: include/llvm/Target/TargetLowering.h:771
+
+    if (NumClusters < 2 || NumClusters < getMinimumJumpTableEntries())
+      return false;
----------------
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.


================
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 {
----------------
hans wrote:
> This is used in isSuitableForBitTests, but does it really need to be exposed in the TargetLowering interface?
This is also used in findBitTestClusters and buildBitTests(), and for me doing this check through TLI doesn't seem to be weird. Please let me know if you see any better place for this function.


================
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();
----------------
hans wrote:
> What's the difference between Range and JumpTableSize? As far as I can tell, those are always the same.
Yes, these are 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;
----------------
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 +=.


https://reviews.llvm.org/D31085





More information about the llvm-commits mailing list