[llvm-commits] [llvm] r144527 - in /llvm/trunk: include/llvm/CodeGen/MachineBranchProbabilityInfo.h lib/CodeGen/MachineBranchProbabilityInfo.cpp

[Andrew Trick]

On Nov 14, 2011, at 1:32 AM, Chandler Carruth wrote:

> On Mon, Nov 14, 2011 at 1:11 AM, Benjamin Kramer <benny.kra at googlemail.com> wrote:
> On 14.11.2011, at 10:00, Chandler Carruth <chandlerc at gmail.com> wrote:
> 
> > Author: chandlerc
> > Date: Mon Nov 14 02:55:59 2011
> > New Revision: 144527
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=144527&view=rev
> > Log:
> > Reuse the logic in getEdgeProbability within getHotSucc in order to
> > correctly handle blocks whose successor weights sum to more than
> > UINT32_MAX. This is slightly less efficient, but the entire thing is
> > already linear on the number of successors. Calling it within any hot
> > routine is a mistake, and indeed no one is calling it. It also
> > simplifies the code.
> 
> Does this problem also exist in the non-machine BPI version of getHotSucc?
> 
> At a fundamental level, yes. But I'm not sure we really care.
> 
> At the IR level, we *compute* the branch probabilities. If the structure of the program is changed radically, we *recompute* them rather than updating them in-place. The computation (in the BPI analysis pass) is careful to eagerly scale weights down such that the sum doesn't overflow. So the assertion (and the lack of code to handle it) seem fine there.
> 
> At the machine code level, we are updating the weights of the edges in the few places where they are mutated. That's what causes trouble. My suspicion is critical edge splitting, but I've not dug too deeply into it.
> 
> So essentially, I don't think the IR-level code is really subject to the bug, despite having the same inability to cope with overflow.

Assuming the sum of edges doesn't overflow is not a bug, it's a very deliberate design feature. Preserving the analysis across CFG updates wasn't implemented, and the "bug" that Chandler is suffering from is that we enabled this code implying it was sufficiently implemented without any safeguards or even documenting the limitations.

Scaling the edges was part of the original design, but it was supposed to happen in setEdgeWeight, not getEdgeWeight. The weights are currently only set by the static estimator, so we didn't need an intelligent API. For general CFG manipulation, we definitely need a wrapper around setEdgeWeight that scales the edges automatically.

-Andy

> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111115/45d29794/attachment.html>