[llvm-commits] Machine Branch Probability Info

[Jakob Olesen]

On Jun 14, 2011, at 2:12 PM, Jakub Staszak wrote:

> 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 :)

Lots of headers do that. It shouldn't be a problem.

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

Yes.

>> @@ -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?

Yes.

Thanks, Jakub.

/jakob