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

[Hans Wennborg via Phabricator via llvm-commits]

hans added inline comments.


================
Comment at: include/llvm/Target/TargetLowering.h:771
+
+    if (NumClusters < 2 || NumClusters < getMinimumJumpTableEntries())
+      return false;
----------------
junbuml wrote:
> hans wrote:
> > 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.
> If we use range for this check in isSuitableForJumpTable(), it will change the behavior in lowering. Current lowering code use  the total number of cluster (N) for this check only once at the beginning of findJumpTables(), which is different from both Range and NumCases. And also when partitioning clusters for jump tables, doing this check in isSuitableForJumpTable() may change the behavior of the loop finding partitions. If either Range or NumCases should be used for this check, I think it should be a separate patch.   
Oh, I see what you mean. 

It's annoying that we'll end up with a `isSuitableForJumpTable()` that doesn't take the minimum size into account though :-/


https://reviews.llvm.org/D31085