From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7901 invoked by alias); 7 May 2019 12:56:45 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 7886 invoked by uid 89); 7 May 2019 12:56:44 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.4 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-lj1-f194.google.com Received: from mail-lj1-f194.google.com (HELO mail-lj1-f194.google.com) (209.85.208.194) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 07 May 2019 12:56:42 +0000 Received: by mail-lj1-f194.google.com with SMTP id q10so14241705ljc.6 for ; Tue, 07 May 2019 05:56:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=a5SeiOk+Qy5u2rCfxz+Qq0RzHWfZClJ5jhyWSoHdIJ4=; b=FfCfulMq9Dr44QdINUkwNOJkgOmRNFW1GXzc++kCpk1ISWI/AY2SMasOESoDdI7Sor +Woa90X+PvQbS9HMtxhhoFKRr9YwdnyweGWFhXblw9pw8quCsAC9TONP9zNOE0vr4wGL +lNXb5xxuMkqgWu0TEC0Qikb5n7u9rqyc9DXRCq7gDifxrBv/qF6Hus6zsuPthK3bo4b ueukiymvS/+hQavNcHsf8EnknOgWu+jncl+ISQR/oAcrIKspn1tXI59H5MNJyYVKywRJ 1FWnZEtwlCWD07CPZ7/Wis9w4j64JaB9wAMJnzFdT9PDRd+gn90RgwIAfuGXjzAcSR+9 ReHA== MIME-Version: 1.0 References: <059b0144-7bb8-eeeb-785c-ee6ebe35a493@suse.cz> <430524fd-7f21-ddd4-1ff2-8e80fb75e01f@suse.cz> <20190409131922.bwal5ts6zxb3irfj@kam.mff.cuni.cz> <31111652-ea81-ff28-6193-8d8577c928eb@suse.cz> <5de0d22e-f244-ed53-1d3c-55e817971daa@suse.cz> <73a2869a-953e-8ac3-2ade-9fcfe61e25c1@suse.cz> <687ffbf1-1e03-81d5-a38d-f699d88a96e6@suse.cz> In-Reply-To: <687ffbf1-1e03-81d5-a38d-f699d88a96e6@suse.cz> From: Richard Biener Date: Tue, 07 May 2019 12:56:00 -0000 Message-ID: Subject: Re: [PATCH][stage1] Support profile (BB counts and edge probabilities) in GIMPLE FE. To: =?UTF-8?Q?Martin_Li=C5=A1ka?= Cc: "Joseph S. Myers" , Jan Hubicka , GCC Patches Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2019-05/txt/msg00296.txt.bz2 On Tue, May 7, 2019 at 2:00 PM Martin Li=C5=A1ka wrote: > > On 5/6/19 4:02 PM, Richard Biener wrote: > > On Mon, May 6, 2019 at 9:59 AM Martin Li=C5=A1ka wrote: > >> > >> On 5/2/19 2:31 PM, Richard Biener wrote: > >>> On Mon, Apr 29, 2019 at 2:51 PM Martin Li=C5=A1ka wr= ote: > >>>> > >>>> On 4/26/19 3:18 PM, Richard Biener wrote: > >>>>> On Wed, Apr 10, 2019 at 10:12 AM Martin Li=C5=A1ka = wrote: > >>>>>> > >>>>>> On 4/9/19 3:19 PM, Jan Hubicka wrote: > >>>>>>>> Hi. > >>>>>>>> > >>>>>>>> There's updated version that supports profile quality for both c= ounts > >>>>>>>> and probabilities. I'm wondering whether ENTRY and EXIT BBs need= s to > >>>>>>>> have set probability. Apparently, I haven't seen any verifier th= at > >>>>>>>> would complain. > >>>>>>> > >>>>>>> Well, you do not need to define it but then several cases will > >>>>>>> degenerate. In particular BB frequencies (for callgraph profile or > >>>>>>> hot/cold decisions) are calculated as ratios of entry BB and give= n BB > >>>>>>> count. If entry BB is undefined you will get those undefined and > >>>>>>> heuristics will resort to conservative answers. > >>>>>>> > >>>>>>> I do not think we use exit block count. > >>>>>>> > >>>>>>> Honza > >>>>>>> > >>>>>> > >>>>>> Thank you Honza for explanation. I'm sending version of the patch > >>>>>> that supports entry BB count. > >>>>>> > >>>>>> I've been testing the patch right now. > >>>>> > >>>>> Can you move the GIMPLE/RTL FE specific data in c_declspecs to > >>>>> a substructure accessed via indirection? I guess enlarging that > >>>>> isn't really what we should do. You'd move gimple_or_rtl_pass > >>>>> there and make that pointer one to a struct aux_fe_data > >>>>> (lifetime managed by the respective RTL/GIMPLE FE, thus > >>>>> to be freed there)? Joseph, do you agree or is adding more > >>>>> stuff to c_declspecs OK (I would guess it could be a few more > >>>>> elements in the future). > >>>> > >>>> Let's wait here for Joseph. > >>> > >>> So looks like it won't matter so let's go with the current approach > >>> for the moment. > >>> > >>>>> > >>>>> -c_parser_gimple_parse_bb_spec (tree val, int *index) > >>>>> +c_parser_gimple_parse_bb_spec (tree val, gimple_parser &parser, > >>>>> + int *index, profile_probability *pro= bablity) > >>>>> { > >>>>> > >>>>> so this will allow specifying probability in PHI node arguments. > >>>>> I think we want to split this into the already existing part > >>>>> and a c_parser_gimple_parse_bb_spec_with_edge_probability > >>>>> to be used at edge sources. > >>>> > >>>> Yes, that's a good idea! > >>>> > >>>>> > >>>>> + if (cfun->curr_properties & PROP_cfg) > >>>>> + { > >>>>> + update_max_bb_count (); > >>>>> + set_hot_bb_threshold (hot_bb_threshold); > >>>>> + ENTRY_BLOCK_PTR_FOR_FN (cfun)->count =3D entry_bb_count; > >>>>> > >>>>> I guess the last one should be before update_max_bb_count ()? > >>>> > >>>> Same here. > >>>> > >>>>> > >>>>> + } > >>>>> > >>>>> + /* Parse profile: quality(value) */ > >>>>> else > >>>>> { > >>>>> - c_parser_error (parser, "unknown block specif= ier"); > >>>>> - return return_p; > >>>>> + tree q; > >>>>> + profile_quality quality; > >>>>> + tree v =3D c_parser_peek_token (parser)->valu= e; > >>>>> > >>>>> peek next token before the if/else and do > >>>>> > >>>>> else if (!strcmp (...)) > >>>>> > >>>>> as in the loop_header case. I expected we can somehow share > >>>>> parsing of profile quality and BB/edge count/frequency? How's > >>>>> the expected syntax btw, comments in the code should tell us. > >>>>> I'm guessing it's quality-id '(' number ')' and thus it should be > >>>>> really shareable between edge and BB count and also __GIMPLE > >>>>> header parsing? So parse_profile_quality should be > >>>>> parse_profile () instead, resulting in a combined value > >>>>> (we do use the same for edge/bb?). > >>>> > >>>> It's problematic, there are different error messages for count/frequ= ency. > >>>> Moreover call to parse_profile_quality in c_parser_gimple_or_rtl_pas= s_list > >>>> is a way how to test that next 'token' is a profile count. > >>> > >>> Who cares about error messages... But sure, I'm just proposing to > >>> merge testing for next token and actual parsing. > >> > >> After I've done removal of hot_bb_threshold parsing, there are just 2 = usages > >> of parse_profile_quality. I would like to leave it as it, not introduc= ing a wrappers. > >> > >>> > >>>>> > >>>>> + else if (!strcmp (op, "hot_bb_threshold")) > >>>>> + { > >>>>> > >>>>> I'm not sure about this - it doesn't make sense to specify this > >>>>> on a per-function base since it seems to control a global > >>>>> variable (booo!)? > >>>> > >>>> Yep, shame on me! > >>>> > >>>>> Isn't this instead computed on-demand > >>>>> based on profile_info->sum_max? > >>>> > >>>> No it's a global value shared among functions. > >>>> > >>>>> If not then I think > >>>>> we need an alternate way of funneling in global state into > >>>>> the GIMPLE FE. > >>>> > >>>> What about --param gimple-fe-hot-bb-threshold ? > >>> > >>> I thought about that, yes ... in absence can it actually be > >>> "computed"? > >> > >> Renamed to it. > >> > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > >> > >> Ready to be installed? > > > > @@ -470,7 +576,8 @@ c_parser_gimple_compound_statement (gimple_parser > > &parser, gimple_seq *seq) > > last_basic_block_for_fn (cfun) =3D index + 1; > > n_basic_blocks_for_fn (cfun)++; > > if (!parser.current_bb) > > - parser.push_edge (ENTRY_BLOCK, bb->index, EDGE_FALLTHRU= ); > > + parser.push_edge (ENTRY_BLOCK, bb->index, EDGE_FALLTHRU, > > + profile_probability::uninitialized ()= ); > > > > /* We leave the proper setting to fixup. */ > > struct loop *loop_father =3D loops_for_fn (cfun)->tree_ro= ot; > > > > the fallthru edge from ENTRY to the single-succ of ENTRY should have > > 100% probability and be "known", no? Or do we actually use uninitializ= ed > > for fallthru edges? > > Fixed by setting to ::always (). > > > > > diff --git a/gcc/params.def b/gcc/params.def > > index 3c9c5fc0f13..840b81f43cc 100644 > > --- a/gcc/params.def > > +++ b/gcc/params.def > > @@ -1414,6 +1414,12 @@ DEFPARAM(PARAM_LOOP_VERSIONING_MAX_OUTER_INSNS, > > " loops.", > > 100, 0, 0) > > > > +DEFPARAM(PARAM_GIMPLE_FE_COMPUTED_HOT_BB_THRESHOLD, > > + "gimple-fe-computed-hot-bb-threshold", > > + "The number of executions of a basic block which is considered= hot." > > + " The parameters is used only in GIMPLE FE.", > > + 0, 0, 0) > > + > > /* > > > > Local variables: > > diff --git a/gcc/predict.c b/gcc/predict.c > > index b010c20ff7d..12a484a799a 100644 > > --- a/gcc/predict.c > > +++ b/gcc/predict.c > > @@ -132,8 +132,11 @@ get_hot_bb_threshold () > > { > > if (min_count =3D=3D -1) > > { > > - min_count > > - =3D profile_info->sum_max / PARAM_VALUE (HOT_BB_COUNT_FRACTION); > > + if (flag_gimple) > > + min_count =3D PARAM_VALUE (PARAM_GIMPLE_FE_COMPUTED_HOT_BB_THRE= SHOLD); > > + else > > + min_count > > + =3D profile_info->sum_max / PARAM_VALUE (HOT_BB_COUNT_FRACTIO= N); > > if (dump_file) > > fprintf (dump_file, "Setting hotness threshold to %" PRId64 ".\= n", > > min_count); > > > > Ick. I'd rather do this somewhere in the frontend where PARAM_VALUE > > is initialized > > via set_hot_bb_threshold. Even redundantly for each parsed GIMPLE > > function would > > be OK and better IMHO. > > I've done that in c_parser_parse_gimple_body. > > > That way you don't need get_current_hot_bb_threshold(?). > > We still want it for dump_function_to_file that prints the value in a com= ment. But that can use the existing get_hot_bb_threshold since we never want to dump -1 in case min_count was never initialized. > > > > OK with that changes. > > > > We need to think about switch stmt parsing where we currently have > > > > switch (argc_2(D)) {default: L2; case 0: L0; case 1: L0; case 2: L1; } > > > > __BB(3): > > L0: > > a_4 =3D 0; > > goto __BB6; > > ... > > > > extending this to > > > > switch (argc_2(D)) { default: L2 (guessed(10)); ... > > > > might be possible so we have profile on the edges. > > I can work on that during this stage1. > > Martin > > > > > After the patch all -gimple dumps have profile - probably good > > but also visually disturbing. Ah well. > > > > Hope to do loop flags as followup soon. > > > > Thanks, > > Richard. > > > >> Thanks, > >> Martin > >> > >>> > >>> Richard. > >>> > >>>> Thanks, > >>>> Martin > >>>> > >>>>> > >>>>> Thanks, > >>>>> Richard. > >>>>> > >>>>> > >>>>>> > >>>>>> Martin > >>>> > >> >