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

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: lib/Analysis/InlineCost.cpp:1006
 
-  // Otherwise, we need to accumulate a cost proportional to the number of
-  // distinct successor blocks. This fan-out in the CFG cannot be represented
-  // for free even if we can represent the core switch as a jumptable that
-  // takes a single instruction.
+  // Otherwise, we assume the most general case where the big swith is lowered
+  // into a balanced binary tree consisting of case clusters, the probability of
----------------
haicheng wrote:
> I think we can exit early if the number of cases is too large.
In SwitchCaseClusterFinder::getEstimatedNumberOfCluster(), we have early exit for a large number of cases. But I guess you mean something else. Can you specify little bit more about the "too large". 


================
Comment at: lib/Analysis/InlineCost.cpp:1015
   // does not (yet) fire.
-  SmallPtrSet<BasicBlock *, 8> SuccessorBlocks;
-  SuccessorBlocks.insert(SI.getDefaultDest());
-  for (auto I = SI.case_begin(), E = SI.case_end(); I != E; ++I)
-    SuccessorBlocks.insert(I.getCaseSuccessor());
-  // Add cost corresponding to the number of distinct destinations. The first
-  // we model as free because of fallthrough.
-  Cost += (SuccessorBlocks.size() - 1) * InlineConstants::InstrCost;
+  int NumCaseCluster = TTI.getEstimatedNumberOfCaseClusters(SI);
+  SmallVector<unsigned, 4> SwitchWorkList;
----------------
haicheng wrote:
> If the estimation chooses to use jumptable, I think we also need to add the cost of the table which is proportional to the range. 
I'm not sure if we really need to consider  the size of table as a cost. I think just couple of instructions to look up the table and jump to actual blocks need to be considered as cost.


================
Comment at: lib/CodeGen/SelectionDAG/SwitchCaseCluster.cpp:86
+  APInt MinCaseVal = MaxCaseVal;
+  for (auto I = SI.case_begin(), E = SI.case_end(); I != E; ++I) {
+    const APInt &CaseVal = I.getCaseValue()->getValue();
----------------
haicheng wrote:
> We can start from begin()+1
Thanks. I will do that.


================
Comment at: test/Transforms/Inline/switch.ll:3
 ; RUN: opt < %s -passes='cgscc(inline)' -inline-threshold=20 -S | FileCheck %s
 
+define i32 @callee1(i32 %a) {
----------------
haicheng wrote:
> We may need to add tests for jump table and bit test.
Yes. I will do that.


https://reviews.llvm.org/D31085