[llvm-commits] Machine Branch Probability Info

[Jakob Olesen]

On Jun 13, 2011, at 11:37 AM, Jakub Staszak wrote:

> This patch introduces MachineBranchProbabilityInfo class, which has similar API to BranchProbabilityInfo (except setEdgeWeight which is not available here). Branch Weights are keep in MachineBasicBlock. They have the same order (and length) as Successors vector. You can turn off this analysis using -use-mbpi=false. Comments are welcome.

Hi Jakub,

--- include/llvm/Support/BranchProbability.h	(revision 132920)
+++ include/llvm/Support/BranchProbability.h	(working copy)
@@ -15,10 +15,10 @@
 #define LLVM_SUPPORT_BRANCHPROBABILITY_H
 
 #include "llvm/Support/DataTypes.h"
+#include "llvm/Support/raw_ostream.h"

Please don't do this, just forward-declare the class. (It's the only change to this header??)

+++ include/llvm/CodeGen/MachineBasicBlock.h	(working copy)

Please make sure that the weight vectors are not allocated in -O0 builds. We don't want to pay for profiling info that isn't there. Same thing with -use-mbpi.

+  unsigned            weight_size() const { return Weights.size();       }

Doesn't this always return the same as succ_size()? Why is this needed?

@@ -248,7 +257,7 @@
   /// addSuccessor - Add succ as a successor of this MachineBasicBlock.
   /// The Predecessors list of succ is automatically updated.
   ///
-  void addSuccessor(MachineBasicBlock *succ);
+  void addSuccessor(MachineBasicBlock *succ, uint32_t weight = 0);
 
Please provide a comment explaining what the weight parameter is and what happens when  it is omitted.

I would also like to have a replaceSuccessor() method that preserves the weight. That is useful for critical edge splitting.

+  /// from MachineBarnchProbabilityInfo class.

Typo

--- lib/CodeGen/MachineVerifier.cpp	(revision 132920)
+++ lib/CodeGen/MachineVerifier.cpp	(working copy)
@@ -538,6 +538,9 @@
 
   if (Indexes)
     lastIndex = Indexes->getMBBStartIdx(MBB);
+
+  // Verify Successor Weighs
+  assert(MBB->succ_size() == MBB->weight_size() && "Async Weight List!");
 }
 
This doesn't need to go in machine verifier. You can simply assert that the vectors are consistent before you use them.

   // Update the successor information.
+  succ_iterator SI = std::find(Successors.begin(), Successors.end(), Old);
+  weight_iterator WI = getWeightIterator(SI);
+  uint32_t weight = *WI;
   removeSuccessor(Old);
-  addSuccessor(New);
+  addSuccessor(New, weight);
 
This is what replaceSuccessor() is for.

   // Update successor info
-  SwitchBB->addSuccessor(CB.TrueBB);
-  SwitchBB->addSuccessor(CB.FalseBB);
+  uint32_t trueWeight = getEdgeWeight(SwitchBB, CB.TrueBB);
+  uint32_t falseWeight = getEdgeWeight(SwitchBB, CB.FalseBB);
 
+  SwitchBB->addSuccessor(CB.TrueBB, trueWeight);
+  SwitchBB->addSuccessor(CB.FalseBB, falseWeight);
+
 
This code is repeated over and over again. Please put it in a function.

/jakob