[llvm] r183174 - Fix a defect in code-layout pass, improving Benchmarks/Olden/em3d/em3d by about 30%

[Shuxin Yang]

Author: shuxin_yang
Date: Mon Jun  3 20:00:57 2013
New Revision: 183174

URL: http://llvm.org/viewvc/llvm-project?rev=183174&view=rev
Log:
Fix a defect in code-layout pass, improving Benchmarks/Olden/em3d/em3d by about 30%
(4.58s vs 3.2s on an oldish Mac Tower). 

  The corresponding src is excerpted bellow. The lopp accounts for about 90% of execution time.
  --------------------
    cat -n test-suite/MultiSource/Benchmarks/Olden/em3d/make_graph.c
     90 
     91         for (k=0; k<j; k++)
     92           if (other_node == cur_node->to_nodes[k]) break;

  The defective layout is sketched bellow, where the two branches need to swap.
  ------------------------------------------------------------------------
      L:
         ...
      if (cond) goto out-of-loop
      goto L

  While this code sequence is defective, I don't understand why it incurs 1/3 of 
execution time. CPU-event-profiling indicates the poor laoyout dose not increase
in br-misprediction; it dosen't increase stall cycle at all, and it dosen't 
prevent the CPU detect the loop (i.e. Loop-Stream-Detector seems to be working fine
as well)... 

   The root cause of the problem is that the layout pass calls AnalyzeBranch() 
with basic-block which is not updated to reflect its current layout.

rdar://13966341

Modified:
    llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp

Modified: llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp?rev=183174&r1=183173&r2=183174&view=diff
==============================================================================
--- llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp (original)
+++ llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp Mon Jun  3 20:00:57 2013
@@ -991,6 +991,28 @@ void MachineBlockPlacement::buildCFGChai
     Cond.clear();
     MachineBasicBlock *TBB = 0, *FBB = 0; // For AnalyzeBranch.
     if (!TII->AnalyzeBranch(*PrevBB, TBB, FBB, Cond)) {
+      // The "PrevBB" is not yet updated to reflect current code layout, so,
+      //   o. it may fall-through to a block without explict "goto" instruction
+      //      before layout, and no longer fall-through it after layout; or 
+      //   o. just opposite.
+      // 
+      // AnalyzeBranch() may return erroneous value for FBB when these two
+      // situations take place. For the first scenario FBB is mistakenly set
+      // NULL; for the 2nd scenario, the FBB, which is expected to be NULL,
+      // is mistakenly pointing to "*BI".
+      //
+      bool needUpdateBr = true;
+      if (!Cond.empty() && (!FBB || FBB == *BI)) {
+        PrevBB->updateTerminator();
+        needUpdateBr = false;
+        Cond.clear();
+        TBB = FBB = 0;
+        if (TII->AnalyzeBranch(*PrevBB, TBB, FBB, Cond)) {
+          // FIXME: This should never take place.
+          TBB = FBB = 0;
+        }
+      }
+
       // If PrevBB has a two-way branch, try to re-order the branches
       // such that we branch to the successor with higher weight first.
       if (TBB && !Cond.empty() && FBB &&
@@ -1003,8 +1025,10 @@ void MachineBlockPlacement::buildCFGChai
         DebugLoc dl;  // FIXME: this is nowhere
         TII->RemoveBranch(*PrevBB);
         TII->InsertBranch(*PrevBB, FBB, TBB, Cond, dl);
+        needUpdateBr = true;
       }
-      PrevBB->updateTerminator();
+      if (needUpdateBr)
+        PrevBB->updateTerminator();
     }
   }