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

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
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();
----------------
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.





https://reviews.llvm.org/D31085