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

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:773
                                ArrayRef<const Value *> Arguments) = 0;
+  virtual int getEstimatedNumberOfCaseClusters(const SwitchInst &SI,
+                                               unsigned &JTSize) = 0;
----------------
hans wrote:
> The other function here return an estimated cost for lowering. I think that would be a better interface for this too.
I doubt if this is good place to get the inline cost because other functions handle  user costs which is different from inline cost. Mixing the user cost and inline cost here might be a bad  choice. I believe the inline cost should be decided in InlineCost. 


================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:175
+  int getEstimatedNumberOfCaseClusters(const SwitchInst &SI,
+                                       unsigned &JumpTableSize) {
+    /// Try to find the estimated number of clusters. Note that the number of
----------------
hans wrote:
> I wish this could be much simpler. Maybe most of the code could defer to TLI::isSuitableForBitTest / isSuitableForJumpTable which could also be used from the DAG code.
Tried to make it simpler, but  we still need to find Min/MaxValue here without forming CaseClusters and it's also good to check IsJTAllowed early before doing the actual suitability check to avoid iterating  the for loop to find Min/MaxValue in case not allowed.  Please take a look and let me know if there is any part you want to move in either isSuitableForBitTest or isSuitableForJumpTable.


================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:225
+
+    // Check if suitable for a jump table.
+    if (IsJTAllowed) {
----------------
hans wrote:
> If we have TLI::isSuitableForBitTests, maybe we should have isSuitableForJumpTable too, that way we don't have to duplicate as much logic, and that one could do what areJTsAllowed() as well.
Added isSuitableForJumpTable in TLI which used in here and DAG, but keep  areJTsAllowed() as a separate function because areJTsAllowed() need to be checked only once in findJumpTable() in DAG, and  we can also hit the early exit in this function when JT is not allowed. 


================
Comment at: lib/Analysis/InlineCost.cpp:1051
+
+  while (!SwitchWorkList.empty()) {
+    unsigned NumCases = SwitchWorkList.back();
----------------
haicheng wrote:
> haicheng wrote:
> > hans wrote:
> > > Why do we have to do this?
> > > 
> > > The code is basically constructing the tree and throwing it away. It should be possible to compute an estimate for the size of the tree with a closed-form mathematical expression.
> > 
> > If n is the case number, f(n) is the mapping from the case number to the node number of BTree.  The recursion is 
> > f(n) = 1 + f(n/2) + f (n - n/2), when n > 3.  
> > So, f(n) is between n + 2^(log2(n) - 1) - 1 and n + 2^(log2(n)) - 1.  The lower bound is about 1.5n - 1 and the upper bound is about 2n - 1
> The exact equation is
> 
> f(n) = n, n <= 3
> f(n) = n + 2^(log(n) - 1) - 1, n > 3 && 2^log(n) <= n <= 1.5*2^(log(n))
> f(n) = 2n - 2^(log(n)) - 1, n > 3 && 1.5*2^(log(n)) < n < 2^(log(n)+1)
Thanks Haicheng for this. 

If n is a power of 2, the number of node should be n + 2^(log2(n) - 1) - 1. For non-power of 2 cases , the lower bound is n + 2^(floor(log2(n)) - 1) - 1 and the upper bound is n + 2^(ceiling(log2(n)) - 1) - 1.  As a estimation, I think the use of upper bound is  simple and conservative enough.
 


https://reviews.llvm.org/D31085