From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 100354 invoked by alias); 23 May 2017 13:40:17 -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 100330 invoked by uid 89); 23 May 2017 13:40:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy=hubicka@ucw.cz, D*ucw.cz, H*Ad:U*rguenther, Hx-languages-length:4446 X-HELO: mail-it0-f42.google.com Received: from mail-it0-f42.google.com (HELO mail-it0-f42.google.com) (209.85.214.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 23 May 2017 13:40:14 +0000 Received: by mail-it0-f42.google.com with SMTP id o5so19383539ith.1 for ; Tue, 23 May 2017 06:40:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Gcm7+WTpAQ03mTDeMItfpY3isjRM4JeNnAAJhSOZBbE=; b=jdxxGzjcVHsFz1+WJwVt3cmYPWafOx7aLjJS/+dHnmSu7aom2M+2OnMGUtob+VpiOA ehl2yTX8kNTEKYf71rMLlP3KVLZkl+zKSQ2y3Pyeq+HqpZA3BrLcVwnKdHKweBF1ZgON oAsUDuqSZ2wJ4870Mw4j2wtH/HZJtIPIMntXc405XvC+Ygtallf+V1pQ1yC0W28qa2BY 3cnZD8ORQhKt9QIvxG/8tJapKaqTE6Dj4/gE8xgbd03DwiKmepUgUlUc4UlRZCY3CmRc oWTPzVlYOgFI7v/at0CXV8CK0nvBE5uRSVomywU9sdCmQERSsC6ntyJTUhNSOC9Iw8N8 jufw== X-Gm-Message-State: AODbwcAzVA89jll+vKTbnvwTWHBZ3icR/4gGxohYK9ymVkrEVMPGcTh/ M+kJ+jTIVZ5Wq7ocP1RBrX+bCPikoqUT X-Received: by 10.36.185.78 with SMTP id k14mr2993594iti.45.1495546815826; Tue, 23 May 2017 06:40:15 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.25.5 with HTTP; Tue, 23 May 2017 06:40:15 -0700 (PDT) In-Reply-To: <20170519133212.GB36419@kam.mff.cuni.cz> References: <20170519133212.GB36419@kam.mff.cuni.cz> From: Prathamesh Kulkarni Date: Tue, 23 May 2017 13:48:00 -0000 Message-ID: Subject: Re: [RFC] propagate malloc attribute in ipa-pure-const pass To: Jan Hubicka Cc: gcc Patches , Richard Biener Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg01749.txt.bz2 On 19 May 2017 at 19:02, Jan Hubicka wrote: >> >> * LTO and memory management >> This is a general question about LTO and memory management. >> IIUC the following sequence takes place during normal LTO: >> LGEN: generate_summary, write_summary >> WPA: read_summary, execute ipa passes, write_opt_summary >> >> So I assumed it was OK in LGEN to allocate return_callees_map in >> generate_summary and free it in write_summary and during WPA, allocate >> return_callees_map in read_summary and free it after execute (since >> write_opt_summary does not require return_callees_map). >> >> However with fat LTO, it seems the sequence changes for LGEN with >> execute phase takes place after write_summary. However since >> return_callees_map is freed in pure_const_write_summary and >> propagate_malloc() accesses it in execute stage, it results in >> segmentation fault. >> >> To work around this, I am using the following hack in pure_const_write_summary: >> // FIXME: Do not free if -ffat-lto-objects is enabled. >> if (!global_options.x_flag_fat_lto_objects) >> free_return_callees_map (); >> Is there a better approach for handling this ? > > I think most passes just do not free summaries with -flto. We probably want > to fix it to make it possible to compile multiple units i.e. from plugin by > adding release_summaries method... > So I would say it is OK to do the same as others do and leak it with -flto. >> diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c >> index e457166ea39..724c26e03f6 100644 >> --- a/gcc/ipa-pure-const.c >> +++ b/gcc/ipa-pure-const.c >> @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3. If not see >> #include "tree-scalar-evolution.h" >> #include "intl.h" >> #include "opts.h" >> +#include "ssa.h" >> >> /* Lattice values for const and pure functions. Everything starts out >> being const, then may drop to pure and then neither depending on >> @@ -69,6 +70,15 @@ enum pure_const_state_e >> >> const char *pure_const_names[3] = {"const", "pure", "neither"}; >> >> +enum malloc_state_e >> +{ >> + PURE_CONST_MALLOC_TOP, >> + PURE_CONST_MALLOC, >> + PURE_CONST_MALLOC_BOTTOM >> +}; > > It took me a while to work out what PURE_CONST means here :) > I would just call it something like STATE_MALLOC_TOP... or so. > ipa_pure_const is outdated name from the time pass was doing only > those two. >> @@ -109,6 +121,10 @@ typedef struct funct_state_d * funct_state; >> >> static vec funct_state_vec; >> >> +/* A map from node to subset of callees. The subset contains those callees >> + * whose return-value is returned by the node. */ >> +static hash_map< cgraph_node *, vec* > *return_callees_map; >> + > > Hehe, a special case of return jump function. We ought to support those more generally. > How do you keep it up to date over callgraph changes? >> @@ -921,6 +1055,23 @@ end: >> if (TREE_NOTHROW (decl)) >> l->can_throw = false; >> >> + if (ipa) >> + { >> + vec v = vNULL; >> + l->malloc_state = PURE_CONST_MALLOC_BOTTOM; >> + if (DECL_IS_MALLOC (decl)) >> + l->malloc_state = PURE_CONST_MALLOC; >> + else if (malloc_candidate_p (DECL_STRUCT_FUNCTION (decl), v)) >> + { >> + l->malloc_state = PURE_CONST_MALLOC_TOP; >> + vec *callees_p = new vec (vNULL); >> + for (unsigned i = 0; i < v.length (); ++i) >> + callees_p->safe_push (v[i]); >> + return_callees_map->put (fn, callees_p); >> + } >> + v.release (); >> + } >> + > > I would do non-ipa variant, too. I think most attributes can be detected that way > as well. > > The patch generally makes sense to me. It would be nice to make it easier to write such > a basic propagators across callgraph (perhaps adding a template doing the basic > propagation logic). Also I think you need to solve the problem with keeping your > summaries up to date across callgraph node removal and duplications. Thanks for the suggestions, I will try to address them in a follow-up patch. IIUC, I would need to modify ipa-pure-const cgraph hooks - add_new_function, remove_node_data, duplicate_node_data to keep return_callees_map up-to-date across callgraph node insertions and removal ? Also, if instead of having a separate data-structure like return_callees_map, should we rather have a flag within cgraph_edge, which marks that the caller may return the value of the callee ? Thanks, Prathamesh > > Honza