From: Jan Hubicka <hubicka@ucw.cz>
To: Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>
Cc: gcc Patches <gcc-patches@gcc.gnu.org>,
Richard Biener <rguenther@suse.de>
Subject: Re: [RFC] propagate malloc attribute in ipa-pure-const pass
Date: Sat, 07 Oct 2017 19:35:00 -0000 [thread overview]
Message-ID: <20171007182345.GA64513@kam.mff.cuni.cz> (raw)
In-Reply-To: <CAAgBjMkUTQw0LkQT9PkvWxAwjdokMcw7kJP_k3QZf2OSYsX8aQ@mail.gmail.com>
> On 6 October 2017 at 06:04, Jan Hubicka <hubicka@ucw.cz> 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 <prathamesh.kulkarni@linaro.org>
> >>
> >> * 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<cgraph_node *> (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<cgraph_node *> (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?
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
next prev parent reply other threads:[~2017-10-07 18:23 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-15 10:56 Prathamesh Kulkarni
2017-05-16 1:14 ` Jeff Law
2017-05-16 11:48 ` Richard Biener
2017-05-17 21:22 ` Martin Sebor
2017-05-18 7:07 ` Richard Biener
2017-05-19 13:18 ` Jan Hubicka
2017-05-19 13:34 ` Jan Hubicka
2017-05-23 13:48 ` Prathamesh Kulkarni
2017-07-31 18:23 ` Prathamesh Kulkarni
2017-08-08 4:21 ` Prathamesh Kulkarni
2017-08-17 12:55 ` Prathamesh Kulkarni
2017-09-01 2:39 ` Prathamesh Kulkarni
2017-09-15 12:19 ` Prathamesh Kulkarni
2017-09-25 18:13 ` Prathamesh Kulkarni
2017-09-26 0:24 ` Jan Hubicka
2017-09-27 1:11 ` Prathamesh Kulkarni
2017-09-29 19:28 ` Jan Hubicka
2017-10-06 2:16 ` Prathamesh Kulkarni
2017-10-06 13:04 ` Jan Hubicka
2017-10-07 1:46 ` Prathamesh Kulkarni
2017-10-07 19:35 ` Jan Hubicka [this message]
2017-10-07 22:17 ` Prathamesh Kulkarni
2017-10-13 23:34 ` Prathamesh Kulkarni
2017-10-23 9:37 ` Prathamesh Kulkarni
2017-10-24 10:57 ` Jan Hubicka
2017-10-25 11:18 ` Prathamesh Kulkarni
2017-10-25 15:26 ` Jan Hubicka
2017-10-27 10:52 ` Prathamesh Kulkarni
2017-10-27 12:20 ` Jan Hubicka
2017-10-27 12:44 ` Jan Hubicka
2017-10-27 13:00 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171007182345.GA64513@kam.mff.cuni.cz \
--to=hubicka@ucw.cz \
--cc=gcc-patches@gcc.gnu.org \
--cc=prathamesh.kulkarni@linaro.org \
--cc=rguenther@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).