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

[Andrew Trick]

On Oct 18, 2011, at 4:12 PM, Eli Friedman wrote:
>> 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.

Someone could start with manually spot checking some cases where block frequency changes between passes.

To automate things a bit, some passes can be fixed to preserve BranchFrequenyInfo. BranchFrequencyInfo consistency can easily be automatically verified against BranchProbabilityInfo.

You can also keep around partial information and at least verify the regions that should be unaffected. For example, you can invalidate BlockFrequencyInfo for a given loop or interval, rerun frequency analysis and verify just the data points that are still valid.

On a tangent, BranchProbabilityInfo is in theory as easy to update as branch weight metadata. The choice between the two is really a matter of how we prefer other passes to interact/update the profile data.

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

Yes. In theory we could use a generic "expect" intrinsic for other optimizations. But it would be implemented differently--it would use the value at a particular point in the CFG, not define a new value. There would be no reason to remove the intrinsic at all, BranchProbabilityAnalysis could directly detect the intrinsics instead of metadata. DCE et al. should be able to handle "metaintrinsics" anyway. Either way, we don't want a LowerExpectIntrinsics pass.

This would have been a more robust way to implement builtin_expect in isolation, but I think we're focussing on the branch weight framework first since we want it for other sources of profile info.

-Andy