[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:
> junbuml wrote:
> > 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. 
> I was thinking it doesn't need to be used as the inlining cost directly, but exposing some metric the inline cost model could then make use of - that's kind of what we're doing anyway.
> 
> What I don't like about this one is that it feels a little out of place compared to other functions which return an estimate of the cost of lowering an instruction, whereas this seems to return details about the lowering instead.
> 
> I'm not sure how to make this better though.
I see what you meant and I can see functions which return the lowering cost. However, this file also hold functions which expose details about the machine directly to be used in IR-level. For me it seems that exposing this information from TLI to InlineCost still keep the original intention of this class.  I will be happy to hear any better suggestion about the way of exposing this lowering information.  


================
Comment at: lib/Analysis/InlineCost.cpp:1048
+      unsigned Exp = Log2_64_Ceil(NumCaseCluster) - 1;
+      uint64_t NumberOfNonLeaf = ((uint64_t) 1 << (Exp)) - 1;
+      uint64_t ExpectedNumberOfCompare = NumCaseCluster + NumberOfNonLeaf;
----------------
hans wrote:
> Very nice! I think the code can be simplified though, and this will allow us to avoid the rounding from Log2_64_Ceil: (N = NumCaseCluster)
> 
> `NumberOfNonLeaf = 2^(log2(N) - 1) - 1 = 2^log2(N) * 2^-1 - 1 = N/2 - 1`
> Adding NumCaseCluster to that yields `N + N / 2 - 1 = N * 3 / 2 - 1`.
> 
> 
> 
> 
> I kind of wish this is all there was to this change and we didn't need all that other code: quick check if the switch is suitable for a bit test or jump table, otherwise compute a cost based on `N * 3 / 2 - 1`. I don't know if it's possible though.
Thanks Hans for this. Updated it based your comments. 

I think it's difficult to use a closed form which cover all lowering cases, especially in different targets. I believe we need to differentiate each lowering case as the costs varying depending on the lowering cases and targets.


================
Comment at: lib/Analysis/InlineCost.cpp:1053
+      uint64_t SwitchCost = ExpectedNumberOfCompare * 2 * InlineConstants::InstrCost;
+      Cost += std::min((uint64_t)INT_MAX, SwitchCost + (uint64_t) Cost);
+    } else
----------------
hans wrote:
> Can we really run into INT_MAX here? Especially given the Threshold check above?
In very extreme case, it could happen.  For a very large Threshold (e.g., INT_MAX), even though SI.getNumCases() * InlineConstants::InstrCost is smaller than the Threshold. ExpectedNumberOfCompare * 2 * InlineConstants::InstrCost could hit INT_MAX. 


https://reviews.llvm.org/D31085