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

[Hans Wennborg]

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7814
@@ +7813,3 @@
+           "Weights must be non-zero for pivot selection to work.");
+    if (LeftWeight < RightWeight) {
+      LeftWeight += (++Left)->Weight;
----------------
djasper wrote:
> No braces.
Done.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7819
@@ +7818,3 @@
+  }
+  CaseClusterIt PivotCluster = Right;
+  assert(PivotCluster > W.FirstCluster && PivotCluster <= W.LastCluster);
----------------
djasper wrote:
> At this point Left and Right are identical and the element they are pointing it belongs to the larger/heavier weight, right?
No, Left == Right - 1 at this point, and LeftWeight and RightWeight are as equal as possible.

For example, if we have cases with weights {3,4,3,5}, the first two belong on the left side (LeftWeight=7, Left=1) and the other two on the right (RightWeight=8, Right=2).

We use Right as the pivot element since we're doing a < comparison.

I'll try to comment this better, and use better names for the variables.

================
Comment at: test/CodeGen/Generic/MachineBranchProb.ll:61
@@ +60,3 @@
+; CHECK-NOT: Successors
+; CHECK: Successors according to CFG: BB#8(13) BB#9(20)
+}
----------------
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'.)

================
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}
+
----------------
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.

http://reviews.llvm.org/D9318

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