[PATCH] D10979: Update the branch weight metadata in JumpThreading pass.

[Xinliang David Li]

On Thu, Jul 9, 2015 at 2:54 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015-Jul-09, at 14:28, Cong Hou <congh at google.com> wrote:
>>
>> On Tue, Jul 7, 2015 at 4:53 PM, Duncan P. N. Exon Smith
>> <dexonsmith at apple.com> wrote:
>>>
>>>> On 2015-Jul-07, at 13:24, Xinliang David Li <davidxl at google.com> wrote:
>>>>
>>>> On Tue, Jul 7, 2015 at 10:39 AM, Duncan P. N. Exon Smith
>>>> <dexonsmith at apple.com> wrote:
>>>>>
>>>>>> On 2015-Jul-06, at 17:22, Cong Hou <congh at google.com> wrote:
>>>>>>
>>>>>>
>>>>>> congh added reviewers: chandlerc, davidxl.
>>>>>> congh added a subscriber: llvm-commits.
>>>>>>
>>>>>> Currently in JumpThreading pass, the branch weight metadata is not updated after CFG modification. Consider the jump threading on PredBB, BB, and SuccBB. After jump threading, the weight on BB->SuccBB should be adjusted as some of it is contributed by the edge PredBB->BB, which doesn't exist anymore. This patch tries to update the edge weight in metadata on BB->SuccBB by scaling it by 1 - Freq(PredBB->BB) / Freq(BB->SuccBB). Two more analyses (BlockFrequencyInfo and BranchProbabilityInfo) are needed then.
>>>>>
>>>>> Thanks for working on this!
>>>>>
>>>>> Generally you don't need these analyses to keep branch weights
>>>>> up-to-date.  The design premise is that you don't need global
>>>>> information for local updates.
>>>>>
>>>>
>>>> This is true in general, but probably not the case for jump-threading.
>>>> When a new thread is formed (from NewBB to SuccBB), the  profile
>>>> update delta to the original edge BB->SuccBB comes from a different
>>>> edge (Pred->NewBB which is inherited from Pred->BB). Local update by
>>>> only looking at BB seems impossible.
>>>
>>> Ah, I see.   Yes, you need a global view to know predecessor
>>> probabilities.
>>>
>>>>
>>>>> You should be able to calculate the new !prof attachment based on the
>>>>> old ones, without running BFI.  (I'm skeptical of even running BPI -- if
>>>>> there's no !prof attachment on the old block, then you have no real
>>>>> information; what's the benefit in generating a new !prof attachment
>>>>> based on heuristics?  The new CFG will come with its own new
>>>>> heuristics.)
>>>>>
>>>>
>>>> I believe Cong's fix is targeting PGO.
>>>
>>> Cong, I have a few concerns with the current approach.
>>>
>>> Firstly, this requires BFI even when there is no profile data (as David
>>> also noted in one of the Phabricator emails).  I don't think it's
>>> reasonable to require all users of jump threading to run an analysis
>>> that will never be used.  One option would be to `addOptional<>`, but
>>> I'm not sure that'll get you what you want.
>>>
>>> To prevent everyone paying for profile data, we should do the following:
>>> if the basic block in question has branch weights, then (and only then)
>>> retrieve the BFI analysis so they can be updated.  I think you'll either
>>> have to port the analysis retrieval and caching logic from the new
>>> `PassManager` over to `LegacyPassManager`, or wait for it to be used in
>>> tree.
>>
>> It seems at this point there isn't a good way to conditionally add a
>> pass dependency to another one, unless the new PassManager is
>> launched. Right?
>
> Right.  AFAICT, you'll either have to port the logic over to the
> `LegacyPassManager`, or wait for (or help with!) the new one.

If we refactor BPI and BFI code  so that they are simply function pass
wrappers to bpi/bfi utility class, then those utility analysis can be
invoked on demand and conditionally. Is that something worth doing?


>
>>> (On a related point, I doubt users of PGO + jump threading with partial
>>> profiles (e.g., JITs) care enough about fidelity to justify running BFI
>>> for this edge.  IMO, this update should be configurable.)
>>>
>>> Secondly, the current code requires BFI and then immediately invalidates
>>> it, but it should be straightforward to incrementally update (preserve)
>>> BFI here.
>>
>> BFI doesn't provide any interface to incrementally update itself.
>> There isn't a way to directly access and modify the frequencies stored
>> in BlockFrequencyInfoImpl.
>
> Fortunately, we have the source code for BFI ;).  I think this
> should be trivial to add.  IIRC there's some sort of `DenseMap<>`
> with the frequencies there.  Patches welcome!

The incremental update interfaces will be useful in other contexts too.

David


>
>> I think maybe we need to update it for the
>> whole function (though it is expensive)?
>
> Wouldn't you just need to (1) modify `BB` and (2) create `NewBB`?
>