From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 113602 invoked by alias); 12 May 2016 13:51:27 -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 113570 invoked by uid 89); 12 May 2016 13:51:26 -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,WEIRD_QUOTING autolearn=ham version=3.3.2 spammy=1.000, UD:hash, Stores, touched X-HELO: mail-vk0-f67.google.com Received: from mail-vk0-f67.google.com (HELO mail-vk0-f67.google.com) (209.85.213.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 12 May 2016 13:51:15 +0000 Received: by mail-vk0-f67.google.com with SMTP id v68so8608133vka.1 for ; Thu, 12 May 2016 06:51:14 -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=qJTAZx4cdUUdEHyGpW+Gw+EV2lHgULtqrbJG0471iOg=; b=PJnDKs4JHMs94ernU+1hzWXkHDnE0SgcUqnaP4RHRVSKTEYz+YVaEQh95zaCGrvuUY wL7TRqg1gSLS2nbGNLgJmLr8rB2DgNUvW3AIJvf+e39mM64RGoIInr4F5cuf8j0Fwn4D v4Q3P4CjffHk3ZoNQKvhWiQR348ZCJtqo0MboY3WU5DMaBWhje2bDyEBsleRnQbHNB0O hCG39f6ZvQiTLMlWzwwLI7pZU5q0eEDxa/S71ngfqlV6yYw/Hn6+P0PIAOVAC9zPmiR4 hYFOajvxaLE+6KUcLTHFSKmajMW18r0ibggUOVg4EEPo/866x9Pdm5v1F9F3CXbKomv7 7hUw== X-Gm-Message-State: AOPr4FVvKmxy0Mbfrbv/KZweusfIwXGCJ1m1YKCARIZQVy9OKOXic4cIqPCkueHd2Ms3wBXDPU5nv+olUqm0aA== MIME-Version: 1.0 X-Received: by 10.31.166.72 with SMTP id p69mr5006312vke.14.1463061072852; Thu, 12 May 2016 06:51:12 -0700 (PDT) Received: by 10.103.44.148 with HTTP; Thu, 12 May 2016 06:51:12 -0700 (PDT) In-Reply-To: <5734737F.2050003@suse.cz> References: <29780c07dc7da0d8f41aa120665072a4098910d8.1461931011.git.mliska@suse.cz> <572C61B9.6060001@suse.cz> <5734737F.2050003@suse.cz> Date: Thu, 12 May 2016 13:51: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/msg00907.txt.bz2 On Thu, May 12, 2016 at 1:13 PM, Martin Li=C5=A1ka wrote: > 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_cas, > 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_e= xps > 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 makes= 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? > > + As check_GNU_style.sh reported multiple 8 spaces issues in hunks I've t= ouched, 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; > + > > /* Stores EXPR in DATA->inv_expr_tab, and assigns it an inv_expr_id. */ > > -static int > +static iv_inv_expr_ent * > get_expr_id (struct ivopts_data *data, tree expr) We are not returning id any more, maybe rename to record_inv_expr or else. > { > struct iv_inv_expr_ent ent; > @@ -4806,13 +4809,13 @@ get_expr_id (struct ivopts_data *data, tree expr) > ent.hash =3D iterative_hash_expr (expr, 0); > slot =3D data->inv_expr_tab->find_slot (&ent, INSERT); > if (*slot) > - return (*slot)->id; > + return *slot; > > *slot =3D XNEW (struct iv_inv_expr_ent); > (*slot)->expr =3D expr; > (*slot)->hash =3D ent.hash; > (*slot)->id =3D data->max_inv_expr_id++; > - return (*slot)->id; > + return *slot; This could be changed to if (!*slot) { //new and insert } return *slot; > } > > /* Returns the pseudo expr id if expression UBASE - RATIO * CBASE > @@ -4820,10 +4823,10 @@ get_expr_id (struct ivopts_data *data, tree expr) > ADDRESS_P is a flag indicating if the expression is for address > computation. */ > > -static int > +static iv_inv_expr_ent * > get_loop_invariant_expr_id (struct ivopts_data *data, tree ubase, > - tree cbase, HOST_WIDE_INT ratio, > - bool address_p) > + tree cbase, HOST_WIDE_INT ratio, > + bool address_p) Rename function name here too. > { > @@ -5988,9 +5992,9 @@ determine_group_iv_costs (struct ivopts_data *data) > if (group->cost_map[j].depends_on) > bitmap_print (dump_file, > group->cost_map[j].depends_on, "",""); > - if (group->cost_map[j].inv_expr_id !=3D -1) > + if (group->cost_map[j].inv_expr !=3D NULL) > fprintf (dump_file, " inv_expr:%d", > - group->cost_map[j].inv_expr_id); > + group->cost_map[j].inv_expr->id); Dump inv_expr in another column thus it won't appear under depends_on in dump. Also make it preceding depends_on which is a bitmap. While we are on this one before the other two, could you please make this independent so it can be committed after rework? Thanks, bin > > Martin