[PATCH] D10979: Update the branch weight metadata in JumpThreading pass.
Duncan P. N. Exon Smith
dexonsmith at apple.com
Mon Jul 13 16:20:09 PDT 2015
> On 2015-Jul-13, at 14:56, Xinliang David Li <davidxl at google.com> wrote:
>
> Many analysis passes in LLVM are implemented as 'XXXWrapperPass' where
> XXX is the analysis info which is decoupled from FunctionPass. For
> instance,
>
> class DominatorTreeWrapperPass : public Function Pass {
> DominatorTree DT; // <<--- this is where the
> implementation is sitting.
> ..
> };
>
>
> Before you go by this route, please run by Duncan to make sure he is
> ok with this ..
>
SGTM.
> David
>
>
> On Mon, Jul 13, 2015 at 2:39 PM, Cong Hou <congh at google.com> wrote:
>> On Thu, Jul 9, 2015 at 4:16 PM, Xinliang David Li <davidxl at google.com> wrote:
>>> 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?
>>
>> This is a good idea. Do we already have similar passes (or utility
>> classes) in LLVM?
>>
>>
>> Cong
>>
>>>
>>>
>>>>
>>>>>> (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`?
>>>>
More information about the llvm-commits
mailing list