[PATCH] D12418: Distribute the weight on the edge from switch to default statement to edges generated in lowering switch.

[Cong Hou via llvm-commits]

congh marked 3 inline comments as done.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8049
@@ +8048,3 @@
+        uint32_t JumpWeight = I->Weight;
+        uint32_t FallthruWeight = UnhandledWeights + DefaultWeight;
+
----------------
hans wrote:
> I think we spell it Fallthrough elsewhere, so maybe rename to FallthroughWeight?
OK.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8056
@@ +8055,3 @@
+                                              SE = JumpMBB->succ_end();
+             SI != SE; ++SI) {
+          if (*SI == Fallthrough) {
----------------
hans wrote:
> Could probably just do "for (MachineBasicBlock *Succ : JumpMBB->successors())" here.
setSuccWeight() needs an iterator as input and that is why I use iterators in this loop.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8098
@@ +8097,3 @@
+          BTB->Weight += DefaultWeight / 2;
+          BTB->DefaultWeight += DefaultWeight / 2;
+        } else
----------------
hans wrote:
> shouldn't it be -= on the DefaultWeight?
Here BTB->DefaultWeight hasn't been updated by adding DefaultWeight to it (it is initialized with UnhandledWeights which doesn't include DefaultWeight) . So it will share a half of it now.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8100
@@ +8099,3 @@
+        } else
+          BTB->DefaultWeight += DefaultWeight;
+
----------------
hans wrote:
> why do we need this else case?
This is because UnhandledWeights doesn't include DefaultWeight. Or I can do this before this branch and use -= in the true body.

================
Comment at: test/CodeGen/X86/switch-edge-weight.ll:35
@@ +34,3 @@
+; CHECK: BB#0:
+; CHECK: Successors according to CFG: BB#4(65) BB#5(25)
+; CHECK: BB#5:
----------------
davidxl wrote:
> hans wrote:
> > It would be super nice if these tests could have comments explaining why these weights are in fact the correct ones.
> > 
> > I don't know if this is a reasonable expectation or not, just throwing it out there - it would make it easier for someone reading the test to verify what's going on.
> > 
> > In this test it seems we'll have four ranges, {1}, {155-159}, {1134} and {1140}. I think the pivot here will be 1134, so the weight on the left is 10+50+10/2=65 and on the right is 10+10+10/5=25, so that matches my expectations. But why are the weights so low below? Shouldn't the edge from the 155-159 range to sw.bb have more weight?
> The weight distribution makes sense to me.
> 
> BB#5 has as test ' %x == 1140'. Since BB#5's weight is 25 and 1140's weight is 10, so this leaves BB#6 15.
> 
> BB#6 has check %x == 1134. One successor is 10, and the remaining (part of the default) is 5.
I missed one more check that includes the edge weight from switch to 155-159 range. I will add it here and also add comments explaining why we check those numbers.


http://reviews.llvm.org/D12418