From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23342 invoked by alias); 7 Oct 2017 19:35:12 -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 23331 invoked by uid 89); 7 Oct 2017 19:35:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.7 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= X-HELO: mail-wm0-f48.google.com Received: from mail-wm0-f48.google.com (HELO mail-wm0-f48.google.com) (74.125.82.48) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 07 Oct 2017 19:35:09 +0000 Received: by mail-wm0-f48.google.com with SMTP id i124so13757596wmf.3 for ; Sat, 07 Oct 2017 12:35:09 -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=79nmpvZKI1s0wlAYrmbLqk1bVriK1XOFTDsQjJpa408=; b=WCW3+AH5ZFstWacfL9uavHK09mSxmwco1j/a/FIuvW8jxipsuEQr2+L6c3+gCmsQ+c vfmE/RcFYsRAPLjwul5n92Z04y/C733imfl5gcARwzzi/uZDpOicqBC2Y9y1t9u/498Q YDRe0CkHCoVo5c9/WjaAwoOR+x6cOeNP6B9hf7X7HnQo3PREq9FYl/Scc20zc14RfUKB 5LUgyu3zLDCfANYISNaGrNyjdgVSsK5t+rtjvpT/JtLBIenwfeGxmdcdb3zSaBeNBvSS RFlO3bEcjeMbR5gcNObVlsOupjOsINidSD6PnUtJEvwKzy/Mnwav0Qz4yuUlVsV4eJgV irYA== X-Gm-Message-State: AMCzsaUHu95LLDYv4kDgBEyYqmjLUK41Xtezl5BJp5yCfyJRafZwM1Hf NVcHbhWAEr0SJ7veYzOK46vjRrfEKHRc3zjtamNEfw== X-Google-Smtp-Source: AOwi7QBw8+X8LFtTclw+6bmfdMthRWOKCJEM9IgTOBKp9ANzj+ZMjFMd5qGhLFjpM869zi3QbkXcOL8TPN1eSy1unVE= X-Received: by 10.28.229.212 with SMTP id c203mr5195738wmh.57.1507404907241; Sat, 07 Oct 2017 12:35:07 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.128.207 with HTTP; Sat, 7 Oct 2017 12:35:06 -0700 (PDT) In-Reply-To: <20171007182345.GA64513@kam.mff.cuni.cz> References: <20170926002423.GB22198@kam.mff.cuni.cz> <20170929192803.GA92290@kam.mff.cuni.cz> <20171006130409.GB67693@kam.mff.cuni.cz> <20171007182345.GA64513@kam.mff.cuni.cz> From: Prathamesh Kulkarni Date: Sat, 07 Oct 2017 22:17: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-10/txt/msg00426.txt.bz2 On 7 October 2017 at 11:23, Jan Hubicka wrote: >> On 6 October 2017 at 06:04, Jan Hubicka wrote: >> >> Hi Honza, >> >> Thanks for the detailed suggestions, I have updated the patch accordingly. >> >> I have following questions on call_summary: >> >> 1] I added field bool is_return_callee in ipa_call_summary to track >> >> whether the caller possibly returns value returned by callee, which >> >> gets rid of return_callees_map. I assume ipa_call_summary_t::remove() >> >> and ipa_call_summary_t::duplicate() will already take care of handling >> >> late insertion/removal of cgraph nodes ? I just initialized >> >> is_return_callee to false in ipa_call_summary::reset and that seems to >> >> work. I am not sure though if I have handled it correctly. Could you >> >> please check that ? >> > >> > I was actually thinking to introduce separate summary for ipa-pure-const pass, >> > but this seems fine to me too (for one bit definitly more effecient) >> > ipa_call_summary_t::duplicate copies all the fields, so indeed you should be >> > safe here. >> > >> > Also it is possible for functions to be inserted late. Updating of call summaries >> > is currently handled by ipa_fn_summary_t::insert >> >> >> >> 2] ipa_inline() called ipa_free_fn_summary, which made >> >> ipa_call_summaries unavailable during ipa-pure-const pass. I removed >> >> call to ipa_free_fn_summary from ipa_inline, and moved it to >> >> ipa_pure_const::execute(). Is that OK ? >> > >> > Seems OK to me. >> >> >> >> Patch passes bootstrap+test and lto bootstrap+test on x86_64-unknown-linux-gnu. >> >> Verfiied SPEC2k6 compiles and runs without miscompares with LTO >> >> enabled on aarch64-linux-gnu. >> >> Cross-tested on arm*-*-* and aarch64*-*-*. I will additionally test >> >> the patch by building chromium or firefox. >> >> Would it be OK to commit if it passes above validations ? >> >> >> >> Thanks, >> >> Prathamesh >> >> > >> >> > Thanks, >> >> > Honza >> > >> >> 2017-10-05 Prathamesh Kulkarni >> >> >> >> * cgraph.h (set_malloc_flag): Declare. >> >> * cgraph.c (set_malloc_flag_1): New function. >> >> (set_malloc_flag): Likewise. >> >> * ipa-fnsummary.h (ipa_call_summary): Add new field is_return_callee. >> >> * ipa-fnsummary.c (ipa_call_summary::reset): Set is_return_callee to >> >> false. >> >> (read_ipa_call_summary): Add support for reading is_return_callee. >> >> (write_ipa_call_summary): Stream is_return_callee. >> >> * ipa-inline.c (ipa_inline): Remove call to ipa_free_fn_summary. >> >> * ipa-pure-const.c: Add headers ssa.h, alloc-pool.h, symbol-summary.h, >> >> ipa-prop.h, ipa-fnsummary.h. >> >> (malloc_state_e): Define. >> >> (malloc_state_names): Define. >> >> (funct_state_d): Add field malloc_state. >> >> (varying_state): Set malloc_state to STATE_MALLOC_BOTTOM. >> >> (check_retval_uses): New function. >> >> (malloc_candidate_p): Likewise. >> >> (analyze_function): Add support for malloc attribute. >> >> (pure_const_write_summary): Stream malloc_state. >> >> (pure_const_read_summary): Add support for reading malloc_state. >> >> (dump_malloc_lattice): New function. >> >> (propagate_malloc): New function. >> >> (ipa_pure_const::execute): Call propagate_malloc and >> >> ipa_free_fn_summary. >> >> (pass_local_pure_const::execute): Add support for malloc attribute. >> >> * ssa-iterators.h (RETURN_FROM_IMM_USE_STMT): New macro. >> >> >> >> testsuite/ >> >> * gcc.dg/ipa/propmalloc-1.c: New test-case. >> >> * gcc.dg/ipa/propmalloc-2.c: Likewise. >> >> * gcc.dg/ipa/propmalloc-3.c: Likewise. >> >> >> >> diff --git a/gcc/cgraph.c b/gcc/cgraph.c >> >> index 3d0cefbd46b..0aad90d59ea 100644 >> >> --- a/gcc/cgraph.c >> >> +++ b/gcc/cgraph.c >> >> @@ -2530,6 +2530,53 @@ cgraph_node::set_nothrow_flag (bool nothrow) >> >> return changed; >> >> } >> >> >> >> +/* Worker to set malloc flag. */ >> > New line here I guess (it is below) >> >> +static void >> >> +set_malloc_flag_1 (cgraph_node *node, bool malloc_p, bool *changed) >> >> +{ >> >> + if (malloc_p && !DECL_IS_MALLOC (node->decl)) >> >> + { >> >> + DECL_IS_MALLOC (node->decl) = true; >> >> + *changed = true; >> >> + } >> >> + >> >> + ipa_ref *ref; >> >> + FOR_EACH_ALIAS (node, ref) >> >> + { >> >> + cgraph_node *alias = dyn_cast (ref->referring); >> >> + if (!malloc_p || alias->get_availability () > AVAIL_INTERPOSABLE) >> >> + set_malloc_flag_1 (alias, malloc_p, changed); >> >> + } >> >> + >> >> + for (cgraph_edge *e = node->callers; e; e = e->next_caller) >> >> + if (e->caller->thunk.thunk_p >> >> + && (!malloc_p || e->caller->get_availability () > AVAIL_INTERPOSABLE)) >> >> + set_malloc_flag_1 (e->caller, malloc_p, changed); >> >> +} >> >> + >> >> +/* Set DECL_IS_MALLOC on NODE's decl and on NODE's aliases if any. */ >> >> + >> >> +bool >> >> +cgraph_node::set_malloc_flag (bool malloc_p) >> >> +{ >> >> + bool changed = false; >> >> + >> >> + if (!malloc_p || get_availability () > AVAIL_INTERPOSABLE) >> >> + set_malloc_flag_1 (this, malloc_p, &changed); >> >> + else >> >> + { >> >> + ipa_ref *ref; >> >> + >> >> + FOR_EACH_ALIAS (this, ref) >> >> + { >> >> + cgraph_node *alias = dyn_cast (ref->referring); >> >> + if (!malloc_p || alias->get_availability () > AVAIL_INTERPOSABLE) >> >> + set_malloc_flag_1 (alias, malloc_p, &changed); >> >> + } >> >> + } >> >> + return changed; >> >> +} >> >> + >> >> diff --git a/gcc/ipa-fnsummary.h b/gcc/ipa-fnsummary.h >> >> index f50d6806e61..613a2b6f625 100644 >> >> --- a/gcc/ipa-fnsummary.h >> >> +++ b/gcc/ipa-fnsummary.h >> >> @@ -197,7 +197,9 @@ struct ipa_call_summary >> >> int call_stmt_time; >> >> /* Depth of loop nest, 0 means no nesting. */ >> >> unsigned int loop_depth; >> >> - >> >> + /* Indicates whether the caller returns the value of it's callee. */ >> > Perhaps add a comment when it is initialized. >> > Don't you also check that the calle is not captured? In that case I would >> > is_return_callee_uncaptured or so, so someone does not try to use it with >> > different meaning. >> Sorry, I didn't understand "Don't you also check that callee is not captured ?" >> What does capturing mean in this context ? > > Don't you need to know that the returned pointer is new and does not alias > with something else? Yes, malloc_candidate_p enforces that locally by checking the returned pointer is return value of callee and optionally has immediate uses only within comparisons against 0. But whether the returned pointer is new depends on whether callee itself can be annotated with malloc, which is done in propagate_malloc. IIUC pointer returned by a malloc annotated function, does not alias anything else ? Thanks, Prathamesh > > Honza >> >> Thanks, >> Prathamesh >> > >> >> @@ -69,6 +74,15 @@ enum pure_const_state_e >> >> >> >> const char *pure_const_names[3] = {"const", "pure", "neither"}; >> >> >> >> +enum malloc_state_e >> >> +{ >> >> + STATE_MALLOC_TOP, >> >> + STATE_MALLOC, >> >> + STATE_MALLOC_BOTTOM >> >> +}; >> >> + >> >> +const char *malloc_state_names[] = {"malloc_top", "malloc", "malloc_bottom"}; >> > >> > Seems static is missing in all those declarations, please fix it. >> > >> > OK with these changes. Thanks! >> > >> > Honza