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

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:194
+
+    auto CI = SI.case_begin();
+    APInt MaxCaseVal = CI->getCaseValue()->getValue();
----------------
hans wrote:
> 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()) {
>   ...
> }
> ```
Yes, I like this. I will do that.


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


https://reviews.llvm.org/D31085