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

[Hans Wennborg via llvm-commits]

hans added a comment.

Thanks for digging into this!


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

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8051
@@ +8050,3 @@
+
+        // If Fallthrough is a target of the jump table, we evenly distributed
+        // the weight on the edge to Fallthrough to successors of CurMBB.
----------------
s/distributed/distribute/ ?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8056
@@ +8055,3 @@
+                                              SE = JumpMBB->succ_end();
+             SI != SE; ++SI) {
+          if (*SI == Fallthrough) {
----------------
Could probably just do "for (MachineBasicBlock *Succ : JumpMBB->successors())" here.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8094
@@ +8093,3 @@
+        // If the cases in bit test don't form a contiguous range, we evenly
+        // distributed the weight on the edge to Fallthrough to two successors
+        // of CurMBB.
----------------
s/distributed/distribute/

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8098
@@ +8097,3 @@
+          BTB->Weight += DefaultWeight / 2;
+          BTB->DefaultWeight += DefaultWeight / 2;
+        } else
----------------
shouldn't it be -= on the DefaultWeight?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8100
@@ +8099,3 @@
+        } else
+          BTB->DefaultWeight += DefaultWeight;
+
----------------
why do we need this else case?

================
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:
----------------
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?


http://reviews.llvm.org/D12418