[llvm-commits] Machine Branch Probability Info

[Jakub Staszak]

Hello Jakob

On Jun 14, 2011, at 11:28 AM, Jakob Olesen wrote:

> 
> 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"

I couldn't compile it when I used class raw_ostream; instread of #include. I don't know why, ask Andy, he will confirm :)

> 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?
> 
OK, I used that for the MachineVerifier, I will remove that. My question is: how can I verify that length of the Weights vector is fine? assert(Weights.empty() || Weights.size() == Successors.size());   // In case if we don't use Weights at all?

> @@ -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.
so you want MachineBasicBlock::replaceSuccessor(MBB *, MBB *) method, right?

> +  /// 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

Thanks!
- Kuba
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110614/9587254c/attachment.html>