From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8479 invoked by alias); 13 May 2016 09:43:47 -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 8462 invoked by uid 89); 13 May 2016 09:43:46 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=AWL,BAYES_50,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=used_inv_exprs, *ivs, cand_cost, pref X-HELO: mail-vk0-f66.google.com Received: from mail-vk0-f66.google.com (HELO mail-vk0-f66.google.com) (209.85.213.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 13 May 2016 09:43:36 +0000 Received: by mail-vk0-f66.google.com with SMTP id c189so11452475vkb.3 for ; Fri, 13 May 2016 02:43:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-transfer-encoding; bh=UcQpihFQhzWZ7h1MjVpR9Zut7KZqgkq5xBYp3D77FXY=; b=jt1FenZJdUvNKWyo/FFlmOOnd35fIy1igR6rQVcAw2XaRfO1Jfc2bRmH+Bw4jGJ3EB yX+ID4xuJKinQ6T/gC8PrVswfcAOxWwuwDAx+0ikpp80rIE0Lc5CuXwnW8yx4OdcuHG4 vL8X/N3xpWM1M2qENSQ4H+lVrCMRg6B5Hh+fveZWf+z0uLTT4Tf8coRuZVoQzsTwaKm1 CwsdBPElhhK/fhFN9JEu/ms3HFkq9E5TE/mq5JAtGvTqS3cieh4dCRb2w4atdQ38rKcw 4hyTxtHB1zkln75ypN5x5ME3bUlRYEep6KgpPVum4L1Q7oM0JjSqj7MVBVT72APgYl6A 3i2Q== X-Gm-Message-State: AOPr4FX1RHzCq+CmcjaEPYCFrxQVWlH4ZDvNO6y1dBN4tvfnD21XsxBY01TTxL4TVYRiIjtffZtFUTppQR33tw== MIME-Version: 1.0 X-Received: by 10.31.166.72 with SMTP id p69mr7357624vke.14.1463132613763; Fri, 13 May 2016 02:43:33 -0700 (PDT) Received: by 10.103.44.148 with HTTP; Fri, 13 May 2016 02:43:33 -0700 (PDT) In-Reply-To: <5734B256.7080002@suse.cz> References: <29780c07dc7da0d8f41aa120665072a4098910d8.1461931011.git.mliska@suse.cz> <572C61B9.6060001@suse.cz> <5734737F.2050003@suse.cz> <5734B256.7080002@suse.cz> Date: Fri, 13 May 2016 09:43:00 -0000 Message-ID: Subject: Re: [PATCH 3/3] Enhance dumps of IVOPTS From: "Bin.Cheng" To: =?UTF-8?Q?Martin_Li=C5=A1ka?= Cc: Richard Biener , GCC Patches , Jan Hubicka Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-05/txt/msg00951.txt.bz2 On Thu, May 12, 2016 at 5:41 PM, Martin Li=C5=A1ka wrote: > On 05/12/2016 03:51 PM, Bin.Cheng wrote: >> On Thu, May 12, 2016 at 1:13 PM, Martin Li=C5=A1ka wrot= e: >>> On 05/10/2016 03:16 PM, Bin.Cheng wrote: >>>> Another way is to remove the use of id for struct iv_inv_expr_ent once >>>> for all. We can change iv_ca.used_inv_expr and cost_pair.inv_expr_id >>>> to pointers, and rename iv_inv_expr_ent.id to count and use this to >>>> record reference number in iv_ca. This if-statement on dump_file can >>>> be saved. Also I think it simplifies current code a bit. For now, >>>> there are id <-> struct maps for different structures in IVOPT which >>>> make it not straightforward. >>> >>> Hi. >>> >>> I'm sending second version of the patch. I tried to follow your advices= , but >>> because of a iv_inv_expr_ent can simultaneously belong to multiply iv_c= as, >>> putting counter to iv_inv_expr_ent does not works. Instead of that, I've >>> decided to replace used_inv_expr with a hash_map that contains used inv= _exps >>> and where value of the map is # of usages. >>> >>> Further questions: >>> + iv_inv_expr_ent::id can be now removed as it's used just for purpose = of dumps >>> Group 0: >>> cand cost scaled freq compl. depends on >>> 5 2 2.00 1.000 >>> 6 4 4.00 1.001 inv_expr:0 >>> 7 4 4.00 1.001 inv_expr:1 >>> 8 4 4.00 1.001 inv_expr:2 >>> >>> That can be replaced with print_generic_expr, but I think using ids mak= es the dump >>> output more clear. >> I am okay with keeping id. Could you please dump all inv_exprs in a >> single section like >> : >> inv_expr 0: print_generic_expr >> inv_expr 1: ... >> >> Then only dump the id afterwards? >> > > Sure, it would be definitely better: > > The new dump format looks: > > : > inv_expr 0: sudoku_351(D) + (sizetype) S.833_774 * 4 > inv_expr 1: sudoku_351(D) + ((sizetype) S.833_774 * 4 + 1844674407370= 9551580) > inv_expr 2: sudoku_351(D) + ((sizetype) S.833_774 + 72) * 4 > inv_expr 3: sudoku_351(D) + ((sizetype) S.833_774 + 81) * 4 > inv_expr 4: &A.832 + (sizetype) _377 * 4 > inv_expr 5: &A.832 + ((sizetype) _377 * 4 + 18446744073709551612) > inv_expr 6: &A.832 + ((sizetype) _377 + 8) * 4 > inv_expr 7: &A.832 + ((sizetype) _377 + 9) * 4 > > : > Group 0: > cand cost scaled freq compl. depends on > > ... > > Improved to: > cost: 27 (complexity 2) > cand_cost: 11 > cand_group_cost: 10 (complexity 2) > candidates: 3, 5 > group:0 --> iv_cand:5, cost=3D(2,0) > group:1 --> iv_cand:5, cost=3D(4,1) > group:2 --> iv_cand:5, cost=3D(4,1) > group:3 --> iv_cand:3, cost=3D(0,0) > group:4 --> iv_cand:3, cost=3D(0,0) > invariants 1, 6 > invariant expressions 6, 3 > > The only question here is that as used_inv_exprs are stored in a hash_map, > order of dumped invariants would not be stable. Is it problem? It is okay. Only nitpicking on this version. > >>> >>> + As check_GNU_style.sh reported multiple 8 spaces issues in hunks I've= touched, I decided >>> to fix all 8 spaces issues. Hope it's fine. >>> >>> I'm going to test the patch. >>> Thoughts? >> >> Some comments on the patch embedded. >> >>> >>> +/* Forward declaration. */ >> Not necessary. >>> +struct iv_inv_expr_ent; >>> + > > I think it's needed because struct cost_pair uses a pointer to iv_inv_exp= r_ent. I mean the comment, clearly the declaration is self-documented. > @@ -6000,11 +6045,12 @@ iv_ca_set_no_cp (struct ivopts_data *data, struct= iv_ca *ivs, > > iv_ca_set_remove_invariants (ivs, cp->depends_on); > > - if (cp->inv_expr_id !=3D -1) > + if (cp->inv_expr !=3D NULL) > { > - ivs->used_inv_expr[cp->inv_expr_id]--; > - if (ivs->used_inv_expr[cp->inv_expr_id] =3D=3D 0) > - ivs->num_used_inv_expr--; > + unsigned *slot =3D ivs->used_inv_exprs->get (cp->inv_expr); > + --(*slot); > + if (*slot =3D=3D 0) > + ivs->used_inv_exprs->remove (cp->inv_expr); I suppose insertion/removal of hash_map are not expensive? Because the algorithm causes a lot of these operations. > @@ -6324,12 +6368,26 @@ iv_ca_dump (struct ivopts_data *data, FILE *file,= struct iv_ca *ivs) > fprintf (file, " group:%d --> ??\n", group->id); > } > > + bool any_invariant =3D false; > for (i =3D 1; i <=3D data->max_inv_id; i++) > if (ivs->n_invariant_uses[i]) > { > + const char *pref =3D any_invariant ? ", " : " invariants "; > + any_invariant =3D true; > fprintf (file, "%s%d", pref, i); > - pref =3D ", "; > } > + > + if (any_invariant) > + fprintf (file, "\n"); > + To make dump easier to read, we can simply dump invariant variables/expressions unconditionally. Also keep invariant variables and expressions in the same form. const char *pref =3D ""; //... fprintf (file, " invariant variables: " for (i =3D 1; i <=3D data->max_inv_id; i++) if (ivs->n_invariant_uses[i]) { fprintf (file, "%s%d", pref, i); pref =3D ", "; } fprintf (file, "\n"); > + const char *pref =3D " invariant expressions "; > + for (hash_map::iterator it > + =3D ivs->used_inv_exprs->begin (); it !=3D ivs->used_inv_exprs->e= nd (); ++it) > + { > + fprintf (file, "%s%d", pref, (*it).first->id); > + pref =3D ", "; > + } > + > fprintf (file, "\n\n"); > } > Okay with the dump change, you may need to update Changelog entry too. Thanks, bin