[llvm-commits] PATCH: Teach BranchProbabilityInfo to read branch metadata

[Eli Friedman]

On Tue, Oct 18, 2011 at 2:54 PM, Andrew Trick <atrick at apple.com> wrote:
> On Oct 17, 2011, at 1:29 AM, Chandler Carruth wrote:
>
> On Mon, Oct 17, 2011 at 12:32 AM, Chandler Carruth <chandlerc at google.com>
> wrote:
>>
>> This patch teaches the BranchProbabilityInfo pass to use metadata encoded
>> probabilities when present on branch instructions. Switch instructions still
>> need to be handled.
>
> And here is a patch which generalizes this logic to support switch
> instructions. It should be applied on top of the previous patch.
> BTW, Chris mentioned an interest in getting this fixed and potentially
> merged onto the 3.0 release branch, so prompt review would be appreciated.
> =]
> <use-branch-prob-metadata-switches.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
> Chandler,
> Your patches look great to me.  I don't suggest any changes. I just have
> some general comments for the record:
> In calcMetaDataWeight, if we really wanted to recover from overflow, we
> would scale back all edge weights (all by the same divisor) until the
> sum is less than UINT32_MAX. But I don't think this is an expected
> case, so your patch should be fine--it's simpler this way, and seems
> like it will handle the output of LowerExpect.
> As you're aware, the biggest problem with the branch weight metadata
> is CFG transforms invalidating it. You've already fixed the obvious
> swapSuccessors issue. That should at least make it usable, but we
> really have no way to verify this stuff currently.

Yes; Bob and I have discussed verification a bit.  It would really be
nice to have some confidence that passes that manipulate the CFG don't
accidentally mess up probablility/frequency information.

> It's also quite silly for opt to run a LowerExpectIntrinsics
> pass. That's a language-specific front-end implementation hack that
> should be part of the clang driver. Given that we're standardizing on
> metadata for branch weights, we should never see those intrinsics in
> bitcode.

IIRC, the justification for the extra pass was that we want to delay
integrating __builtin_expect into the related branches until after a
few optimization passes because it could open up more opportunities
for using __builtin_expect information.  Not sure if it's really a
good idea in practice.

-Eli