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

[Hans Wennborg via Phabricator via llvm-commits]

hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: include/llvm/Target/TargetLowering.h:787
+  bool isSuitableForJumpTable(const SwitchInst *SI, uint64_t NumCases,
+                              uint64_t Range) const {
+    const bool OptForSize = SI->getParent()->getParent()->optForSize();
----------------
junbuml wrote:
> hans wrote:
> > It still seems backward that this is checking density and maximum table size, but that the minimum size is expected to be checked elsewhere :-/ There must be some better way to factor this.
> I agree that doing minSize check in isSuitableForJumpTable looks clear.  However, I want to keep the changes in lowering side in this patch is NFC. That's why we cannot do this check in isSuitableForJumpTable(). 
> 
> If you want to add MinSize check in isSuitableForJumpTable(), I think we should also pass the cluster size (N) to isSuitableForJumpTable() in findJumpTable() because Range and NumCases could be different from the cluster size(N). 
> 
> Only way I can think of doing this check in isSuitableForJumpTable() is : 
>  1. pass the cluster size (N) to isSuitableForJumpTable() for the first call of isSuitableForJumpTable()  in findJumpTable() because this is for the whole range.
>  2. Since the second calls of isSuitableForJumpTable() in findJumpTable for partitionized clusters should not check MinSize with sub clusters, we do not use isSuitableForJumpTable(), instead checking the max size and density separately just like current code.  
> 
> However, I think this way is even worse than my current code.  Please let me know your thought.
> 
> 
> 
Let's put a FIXME in isSuitableForJumpTable that it would be nice if the min size check could somehow be combined with the other checks here.


https://reviews.llvm.org/D31085