[PATCH] D10717: Enhance loop rotation with existence of profile data in MachineBlockPlacement pass.

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:92
@@ +91,3 @@
+    "misfetch-cost",
+    cl::desc("Cost that models the probabalistic risk of an instruction "
+             "misfetch due to a jump comparing to falling through, whose cost "
----------------
Fix typo: probabilistic.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:99
@@ +98,3 @@
+    "jump-inst-cost",
+    cl::desc(
+        "Relative cost of jump instructions comparing to cost of misfetch."),
----------------
>From the way this parameter is used below, it does not look like a relative cost, but more absolute cost.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:842
@@ +841,3 @@
+      return 0;
+    return Freq / BranchProbability(1, Scale);
+  };
----------------
Why not just multiply with Scale?

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:855
@@ +854,3 @@
+          MBFI->getBlockFreq(Pred) * MBPI->getEdgeProbability(Pred, HeaderBB);
+      auto FallThruFreq = ScaleBlockFrequency(EdgeFreq, MisfetchCost);
+      // If the predecessor has only an unconditional jump to the header, we
----------------
Why is the result called 'Freq' -- it should really be the cost.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:893
@@ +892,3 @@
+  for (auto Iter = LoopChain.begin(), TailIter = std::prev(LoopChain.end()),
+            EndIter = LoopChain.end();
+       Iter != EndIter; Iter++, TailIter++) {
----------------
Fix indentation.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:922
@@ +921,3 @@
+    //   JumpInstCost) * tail node frequency.
+    //   2. If the tail node has two successors, then we still get an additional
+    //   jmp instruction. Note that the more frequent executed jmp instruction
----------------
Fix the comment. It is not 'we still get an additional ..', but 'we may still get an additional jmp if the exit bb does not have its target as the layout successor'.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:925
@@ +924,3 @@
+    //   will be put ahead of the other one. Assume the frequency of those two
+    //   branches are x and y, where x is the frequency of the edge to the chair
+    //   head, then the cost will be (x * MisfetechCost + min(x, y) *
----------------
chair --> chain

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:938
@@ +937,3 @@
+        auto TailToHeadFreq = TailBBFreq * TailToHeadProb;
+        auto ColderEdgeFreq = TailToHeadProb > BranchProbability(1, 2)
+                                  ? TailBBFreq * TailToHeadProb.getCompl()
----------------
does this not consider the case when a direct jump is not needed?


http://reviews.llvm.org/D10717