This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH AutoFDO/4]Fix profile count computation/propagation.
- From: Jeff Law <law at redhat dot com>
- To: "Bin.Cheng" <amker dot cheng at gmail dot com>
- Cc: bin dot cheng at linux dot alibaba dot com, gcc-patches List <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 13 Dec 2018 12:06:30 -0700
- Subject: Re: [PATCH AutoFDO/4]Fix profile count computation/propagation.
- References: <9163e52b-e6d9-4ce8-ad45-590eede580b1.bin.cheng@linux.alibaba.com> <a3d49711-eda2-524b-89bd-7984afbd55ab@redhat.com> <CAHFci29ePpCwwAwtE2atEpT6irHtd8gHsA6rbR3U=sT-c4cqBw@mail.gmail.com>
On 12/10/18 6:36 PM, Bin.Cheng wrote:
> On Thu, Nov 8, 2018 at 6:33 AM Jeff Law <law@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.cheng@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.cheng@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.cheng@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