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

David davidxl at google.com
Fri Jun 26 14:18:41 PDT 2015


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:250
@@ -249,3 +249,3 @@
   void buildLoopChains(MachineFunction &F, MachineLoop &L);
-  void rotateLoop(BlockChain &LoopChain, MachineBasicBlock *ExitingBB,
+  void rotateLoop(BlockChain &LoopChain, MachineLoop &L,
                   const BlockFilterSet &LoopBlockSet);
----------------
Why changing this interface at all? There does not seem to be necessary.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:761
@@ -759,1 +760,3 @@
+  auto F = (*LoopChain.begin())->getParent();
+  MachineBasicBlock *ExitingBB = findBestLoopExit(*F, L, LoopBlockSet);
   if (!ExitingBB)
----------------
ExistingBB is already computed in the caller, there is no need for the interface change here ..

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:797
@@ -793,1 +796,3 @@
 
+/// \brief Attempt to rotate a loop based on profile data to reduce branch cost.
+///
----------------
Though equivalent, but it might be more readable if it computes 'benefit/savings' in terms of the fall through opportunities instead of the cost.  For instance, in the following three items, 1 and 2 are really savings that come with the rotation, and 3 is the saving that will be lost with the candidate layout (including the original layout before the rotation).

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:818
@@ +817,3 @@
+  double HeaderFallThruFreq = 0;
+  for (auto *Pred : HeaderBB->predecessors()) {
+    BlockChain *PredChain = BlockToChain[Pred];
----------------
Why is this loop outside of the following loop? I expect  each new candidate loop header to have a chance to check the fall through opportunity and related saving/cost.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:821
@@ +820,3 @@
+    if (!LoopBlockSet.count(Pred) &&
+        (!PredChain || Pred == *std::prev(PredChain->end()))) {
+      double EdgeFreq = static_cast<double>(
----------------
If the previous chain does not end up connecting with this new top block in the current loop chain, then the benefit of the fall through won't be materializable, for instance when the Pred has more than 1 successors and the other successor is also a chain lead.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:828
@@ +827,3 @@
+      if (Pred->succ_size() == 1)
+        EdgeFreq *= 2;
+      HeaderFallThruFreq = std::max(HeaderFallThruFreq, EdgeFreq);
----------------
I think the adjustment should be done the other way:

when num of successors > 1, the cost will be adjust to the minimal freq of the Pred's outgoing edge -- that is where the unconditional jump will be inserted (assuming the fall through path is the hottest one)

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:846
@@ +845,3 @@
+    // the fall through edge from outside of the loop to the header.
+    if (Iter == HeaderIter)
+      Cost -= HeaderFallThruFreq;
----------------
See the previous comment. Even when Iter is not the current Head, it may still have fall through opportunity.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:855
@@ +854,3 @@
+      if (!LoopBlockSet.count(Succ) &&
+          (!SuccChain || Succ == *SuccChain->begin())) {
+        auto EdgeFreq =
----------------
Do you also want to check if the Succ block has more than one incoming edges. If it has more than one incoming edges, need to make sure 'Succ' block's  other predecessor is not a tail of another chain that can be connnected with 'Succ'.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:870
@@ +869,3 @@
+      // consider the cost of this jump.
+      Cost += (TailBB->succ_size() == 1 ? 2 : 1) *
+              static_cast<double>(FallThruEdgeFreq.getFrequency());
----------------
This needs more explanation.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:939
@@ +938,3 @@
+  // loop by heavily using the profile data for better layout.
+  if (F.getFunction()->getEntryCount())
+    rotateLoopWithProfile(LoopChain, L, LoopBlockSet);
----------------
I suggest adding another intern option to guard this.

if (UseProfileBasedCostAnalysis && F.getFunction()->getEntryCount()) ...

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:941
@@ +940,3 @@
+    rotateLoopWithProfile(LoopChain, L, LoopBlockSet);
+  else if (LoopTop == L.getHeader()) {
+    // If we selected just the header for the loop top, look for a potentially
----------------
the check does not seem necessary. If LoopTop == header, ExitingBB will be null.

http://reviews.llvm.org/D10717

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






More information about the llvm-commits mailing list