[PATCH] Switch lowering: use profile info to build weight-balanced binary search trees

[Daniel Jasper]

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7997
@@ +7996,3 @@
+
+  // Move LastLeft and FirstRight towards eachother from opposite directions to
+  // find a partitioning of the clusters which balances the weight on both
----------------
"each other"?

================
Comment at: test/CodeGen/Generic/MachineBranchProb.ll:61
@@ +60,3 @@
+; CHECK-NOT: Successors
+; CHECK: Successors according to CFG: BB#8(13) BB#9(20)
+}
----------------
hans wrote:
> djasper wrote:
> > Maybe add a comment that this division is because the left half is 1+10+1+1 = 13, the right half is 10+10=20 and we are directly checking against the pivot element so that that one weight doesn't contribute to either side. (If I am correct).
> The first weight in the metadata is for the default case. The weights on the left and right are: 10+1+1+1=13 vs 10+10=20.
> 
> I'll add comments making this easier to read.
> 
> The pivot element is part of the right side and does contribute to that weight. (This is different from gcc which I think always does both 'je' and 'jg' on pivot elements, re-using the condition code from the 'cmp'.)
Right. Why doesn't LLVM do that? Intuitively, it sounds like a good idea. (But that's for a follow-up patch if that).

================
Comment at: test/CodeGen/Generic/MachineBranchProb.ll:58
@@ +57,3 @@
+; Check that we set branch weights on the pivot cmp instruction correctly.
+; Cases {0,10,20,30} go on the left with weight 13; cases {40,50} go on the
+; right with weight 20.
----------------
This is actually an interesting case. As I understand it, LLVM will now do?

  this_case = (x < 40
                   ? x < 20 ? x == 0 ? A : x == 10 ? B : DEFAULT
                            : x == 20 ? C : x == 30 ? D : DEFAULT
                   : x == 40 ? E : x == 50 ? F : DEFAULT);

(I hope it is somewhat readable what I mean, case A represents 0, case B 10,... - I just didn't want to re-use the numbers as that would be even more confusing).

Now, I hope I counted correctly, this gives me the following numbers.

  No of comparisons:
  A:    3 (Weight: 10, Weighted: 30)
  B:    4 (Weight:  1, Weighted:  4)
  C:    3 (Weight:  1, Weighted:  3)
  D:    4 (Weight:  1, Weighted:  4)
  E:    2 (Weight: 10, Weighted: 20)
  F:    3 (Weight: 10, Weighted: 30)
  ---
  SUM: 19 (            Weighted: 91)

However, if we split this equally, we get to do a linear scan on both sides:

  this_case = (x < 30 ? x == 0 ? A : x == 10 ? B : x == 20 ? C : DEFAULT
                      : x == 40 ? E : x == 50 ? F : x == 30 ? D : DEFAULT);

  No of comparisons:
  A:    2 (Weight: 10, Weighted: 20)
  B:    3 (Weight:  1, Weighted:  3)
  C:    4 (Weight:  1, Weighted:  4)
  D:    4 (Weight:  1, Weighted:  4)
  E:    2 (Weight: 10, Weighted: 20)
  F:    3 (Weight: 10, Weighted: 30)
  ---
  SUM: 18 (            Weighted: 81)

Which seems beneficial in total. I think the rule might be something like: Do never create a split with less than three elements on one side unless the smallest weight on that side is larger than all the weights on the other side. But that might not be sufficient.

================
Comment at: test/CodeGen/Generic/MachineBranchProb.ll:70
@@ +69,3 @@
+  i32 1,
+  ; Case 10, 20, 30:
+  i32 10, i32 1, i32 1,
----------------
I think this is Case 0, 10, 20

================
Comment at: test/CodeGen/X86/switch.ll:471
@@ +470,3 @@
+
+!3 = !{!"branch_weights", i32 1, i32 10, i32 0, i32 0, i32 0, i32 0, i32 10}
+
----------------
hans wrote:
> djasper wrote:
> > Could you describe somewhere (patch description, comment) what weight=0 means and how it affects the balancing? From this test, I would assume that we just do balanced trees in this case. However, the test below suggests otherwise.
> The problem is that the pivot finding code cannot handle 0-weight nodes as they don't affect the weight-balance. E.g. a tree with weights {10,0,0,0,0,0,0} to the left and {10} on the right would be considered balanced because the sub-trees have equal weight.
> 
> Luckily, BranchProbability::getEdgeWeight() will never return 0. I just figured this was important enough to have a test for.
> 
> I'll add a comment in the code.
But what's the added benefit over the test below?

http://reviews.llvm.org/D9318

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/