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

[Cong Hou via llvm-commits]

congh marked 3 inline comments as done.

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

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:842
@@ +841,3 @@
+      return 0;
+    return Freq / BranchProbability(1, Scale);
+  };
----------------
davidxl wrote:
> Why not just multiply with Scale?
There is no operator * defined between a BlockFrequency and an integer. Also, this / operator is defined to be saturating. I will add a comment for explanation.

================
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
----------------
davidxl wrote:
> Why is the result called 'Freq' -- it should really be the cost.
I think it is due to some historic reason. Fixed.

================
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++) {
----------------
davidxl wrote:
> Fix indentation.
This is the result from running clang-format.

================
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
----------------
davidxl wrote:
> 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'.
Note that we are sure that the two BBs involved here were layout together before, and the second one is a successor of the first one. So we are sure that there will be an additional jump.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:938
@@ +937,3 @@
+        auto TailToHeadFreq = TailBBFreq * TailToHeadProb;
+        auto ColderEdgeFreq = TailToHeadProb > BranchProbability(1, 2)
+                                  ? TailBBFreq * TailToHeadProb.getCompl()
----------------
davidxl wrote:
> does this not consider the case when a direct jump is not needed?
This should not happen. For this rotation, the TailBB is the tail and its layout successor must be a BB that is outside of the loop chain.


http://reviews.llvm.org/D10717