On 12/10/18 6:36 PM, Bin.Cheng wrote:
> On Thu, Nov 8, 2018 at 6:33 AM Jeff Law <l...@redhat.com> wrote:
>> On 10/31/18 12:34 AM, bin.cheng wrote:
>>> Hi,
>>> This patch fixes AutoFDO breakage on trunk. The main reason for breakage
>>> is AutoFDO
>>> relies on standalone edge count computing and propagating profile
>>> count/probability info
>>> on CFG, but in new infra, edge count is actually computed from probability,
>>> which leads
>>> to chicken-egg problem and corrupted profile count. This patch fixes the
>>> issue by using
>>> explicit edge count.
>>>
>>> There is another issue not touched yet that, in quite common case, profiled
>>> samples are
>>> not enough and profile info computed for lots of blocks is ZERO. In the
>>> future, we may
>>> add some heuristics checking quality of sampled counts and reverting to
>>> guessed profile
>>> count if necessary. I think change made in this patch is also needed for
>>> that.
>>>
>>> Package mysql server is used in test of this patch set. It can't be
>>> compiled with autofdo
>>> on trunk, even with compilation issues worked-around, there isn't
>>> performance improvement.
>>> I local experiments, with this patch set it's improved by 12.3%, 4.3%
>>> irrespectively for
>>> read-only/write-heavy benchmarks. Unfortunately, this patch set was
>>> written against
>>> GCC 8 branch a while ago, improvement gets worse on trunk and I haven't
>>> investigated
>>> the reason yet. I guess there are still other issues which need to be
>>> fixed in the future.
>>>
>>> Bootstrap and test on x86_64 in patch set. Is it OK?
>>>
>>> Thanks,
>>> bin
>>> 2018-10-31 Bin Cheng <bin.ch...@linux.alibaba.com>
>>>
>>> * auto-profile.c (AFDO_EINFO): New macro.
>>> (struct edge_info): New structure.
>>> (is_edge_annotated, set_edge_annotated): Delete.
>>> (afdo_propagate_edge, afdo_propagate_circuit, afdo_propagate): Remove
>>> parameter. Adjust edge count computation and annotation using struct
>>> edge_info.
>>> (afdo_calculate_branch_prob): Ditto.
>>> (afdo_annotate_cfg): Simplify code setting basic block profile count.
>>>
>>>
>>> 0004-Fix-AutoFDO-breakage-after-profile-count-rewriting.patch
>>>
>>> From 6506c12d1b633b6d1bfae839b3633a4f99b3a481 Mon Sep 17 00:00:00 2001
>>> From: chengbin <bin.ch...@linux.alibaba.com>
>>> Date: Mon, 20 Aug 2018 15:25:02 +0800
>>> Subject: [PATCH 4/4] Fix AutoFDO breakage after profile count rewriting.
>>>
>>> ---
>>> gcc/auto-profile.c | 190
>>> ++++++++++++++++++++++++++---------------------------
>>> 1 file changed, 95 insertions(+), 95 deletions(-)
>>>
>>> diff --git a/gcc/auto-profile.c b/gcc/auto-profile.c
>>> index cde4f41c1d9..ff3ea23d830 100644
>>> --- a/gcc/auto-profile.c
>>> +++ b/gcc/auto-profile.c
>>> @@ -101,6 +101,17 @@ along with GCC; see the file COPYING3. If not see
>>> namespace autofdo
>>> {
>>>
>>> +/* Intermediate edge info used when propagating AutoFDO profile
>>> information.
>>> + We can't edge->count() directly since it's computed from edge's
>>> probability
>>> + while probability is yet not decided during propagation. */
>>> +#define AFDO_EINFO(e) ((struct edge_info *) e->aux)
>>> +struct edge_info
>>> +{
>>> + edge_info () : count (profile_count::zero ().afdo ()), annotated_p
>>> (false) {}
>>> + profile_count count;
>>> + bool annotated_p;
>>> +};
>> edge_info isn't POD, so make it a class rather than a struct.
>>
>> OK with that change assuming it does not have a hard dependency on prior
>> patches in this series.
> Thanks very much for review. Now that all prerequisite patches are
> approved and committed, I update this one by making edge_info a class
> as suggested.
> Bootstrap and test as before, is it OK?
>
> Thanks,
> bin
>
> 2018-12-11 Bin Cheng <bin.ch...@linux.alibaba.com>
>
> * auto-profile.c (AFDO_EINFO): New macro.
> (class edge_info): New class.
> (is_edge_annotated, set_edge_annotated): Delete.
> (afdo_propagate_edge, afdo_propagate_circuit, afdo_propagate): Remove
> parameter. Adjust edge count computation and annotation using class
> edge_info.
> (afdo_calculate_branch_prob, afdo_annotate_cfg): Likewise.
OK
jeff