<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>Hello Jakob</div><div><br></div><div>On Jun 14, 2011, at 11:28 AM, Jakob Olesen wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div><br>On Jun 13, 2011, at 11:37 AM, Jakub Staszak wrote:<br><br><blockquote type="cite">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.<br></blockquote><br>Hi Jakub,<br><br>--- include/llvm/Support/BranchProbability.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 132920)<br>+++ include/llvm/Support/BranchProbability.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)<br>@@ -15,10 +15,10 @@<br> #define LLVM_SUPPORT_BRANCHPROBABILITY_H<br><br> #include "llvm/Support/DataTypes.h"<br>+#include "llvm/Support/raw_ostream.h"<br></div></blockquote><div><br></div><div>I couldn't compile it when I used class raw_ostream; instread of #include. I don't know why, ask Andy, he will confirm :)</div><br><blockquote type="cite"><div>Please don't do this, just forward-declare the class. (It's the only change to this header??)<br><br>+++ include/llvm/CodeGen/MachineBasicBlock.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)<br><br>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.<br><br>+ unsigned weight_size() const { return Weights.size(); </div></blockquote><blockquote type="cite"><div>Doesn't this always return the same as succ_size()? Why is this needed?<br><br></div></blockquote>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?<br><br><blockquote type="cite"><div>@@ -248,7 +257,7 @@<br> /// addSuccessor - Add succ as a successor of this MachineBasicBlock.<br> /// The Predecessors list of succ is automatically updated.<br> ///<br>- void addSuccessor(MachineBasicBlock *succ);<br>+ void addSuccessor(MachineBasicBlock *succ, uint32_t weight = 0);<br><br>Please provide a comment explaining what the weight parameter is and what happens when it is omitted.<br><br>I would also like to have a replaceSuccessor() method that preserves the weight. That is useful for critical edge splitting.<br></div></blockquote><div>so you want MachineBasicBlock::replaceSuccessor(MBB *, MBB *) method, right?</div><br><blockquote type="cite"><div>+ /// from MachineBarnchProbabilityInfo class.<br><br>Typo<br><br>--- lib/CodeGen/MachineVerifier.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 132920)<br>+++ lib/CodeGen/MachineVerifier.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)<br>@@ -538,6 +538,9 @@<br><br> if (Indexes)<br> lastIndex = Indexes->getMBBStartIdx(MBB);<br>+<br>+ // Verify Successor Weighs<br>+ assert(MBB->succ_size() == MBB->weight_size() && "Async Weight List!");<br> }<br><br>This doesn't need to go in machine verifier. You can simply assert that the vectors are consistent before you use them.<br><br> // Update the successor information.<br>+ succ_iterator SI = std::find(Successors.begin(), Successors.end(), Old);<br>+ weight_iterator WI = getWeightIterator(SI);<br>+ uint32_t weight = *WI;<br> removeSuccessor(Old);<br>- addSuccessor(New);<br>+ addSuccessor(New, weight);<br><br>This is what replaceSuccessor() is for.<br><br> // Update successor info<br>- SwitchBB->addSuccessor(CB.TrueBB);<br>- SwitchBB->addSuccessor(CB.FalseBB);<br>+ uint32_t trueWeight = getEdgeWeight(SwitchBB, CB.TrueBB);<br>+ uint32_t falseWeight = getEdgeWeight(SwitchBB, CB.FalseBB);<br><br>+ SwitchBB->addSuccessor(CB.TrueBB, trueWeight);<br>+ SwitchBB->addSuccessor(CB.FalseBB, falseWeight);<br>+<br><br>This code is repeated over and over again. Please put it in a function.<br><br>/jakob<font class="Apple-style-span" color="#000000"><font class="Apple-style-span" color="#144FAE"><br></font></font></div></blockquote><br></div><div>Thanks!</div><div>- Kuba</div></body></html>