RE: [PATCH V2] Generalized value pass-through for self-recursive function (ipa/pr93203)
Hi Feng, It looks like the option that causes this to trigger is `--param ipa-cp-eval-threshold=1` So on AArch64 I get it to trigger with -mcpu=native -Ofast -fomit-frame-pointer -flto --param ipa-cp-eval-threshold=1 Kind Regards, Tamar > -Original Message- > From: Feng Xue OS > Sent: Tuesday, February 11, 2020 13:11 > To: Tamar Christina ; Martin Jambor > ; Jan Hubicka ; gcc- > patc...@gcc.gnu.org > Cc: nd > Subject: Re: [PATCH V2] Generalized value pass-through for self-recursive > function (ipa/pr93203) > > Hi Tarmar, > > Since I could not reproduce this ICE with my local testing on Spec2017, > would > you share your config, or command line options extracted from compilation > of perlbench? > > Thanks, > Feng > > > From: gcc-patches-ow...@gcc.gnu.org > on behalf of Tamar Christina > Sent: Tuesday, February 11, 2020 6:05 PM > To: Feng Xue OS; Martin Jambor; Jan Hubicka; gcc-patches@gcc.gnu.org > Cc: nd > Subject: RE: [PATCH V2] Generalized value pass-through for self-recursive > function (ipa/pr93203) > > Hi Feng, > > This patch (commit a0f6a8cb414b687f22c9011a894d5e8e398c4be0) is causing > ICEs in the GCC and perlbench benchmark in Spec2017. > > during IPA pass: cp > lto1: internal compiler error: in find_more_scalar_values_for_callers_subset, > at ipa-cp.c:4709 > 0x1698187 find_more_scalar_values_for_callers_subset > ../.././gcc/ipa-cp.c:4709 > 0x169f7d3 decide_about_value > ../.././gcc/ipa-cp.c:5490 > 0x169fdc3 decide_whether_version_node > ../.././gcc/ipa-cp.c:5537 > 0x169fdc3 ipcp_decision_stage > ../.././gcc/ipa-cp.c:5718 > 0x169fdc3 ipcp_driver > ../.././gcc/ipa-cp.c:5901 > Please submit a full bug report, > > Thanks, > Tamar > > > -Original Message- > > From: gcc-patches-ow...@gcc.gnu.org > On > > Behalf Of Feng Xue OS > > Sent: Monday, February 10, 2020 03:29 > > To: Martin Jambor ; Jan Hubicka ; > > gcc-patches@gcc.gnu.org > > Subject: Re: [PATCH V2] Generalized value pass-through for > > self-recursive function (ipa/pr93203) > > > > >> - gcc_checking_assert (item->value); > > > > > I've been staring at this for quite a while, trying to figure out > > > how your patch can put NULL here before I realized it was just a > > > clean-up > > > :-) Sending such changes independently or pointing them out in the > > > email/ChangeLog makes review easier. > > > > Ok. I'll add some description on this cleanup on ChangeLog. > > > > >> @@ -5564,7 +5610,6 @@ decide_whether_version_node (struct > > cgraph_node *node) > > >> } > > >>clone = create_specialized_node (node, known_csts, > known_contexts, > > >> aggvals, callers); > > >> - info = IPA_NODE_REF (node); > > > > > please either drop this change or change it to: > > > > > > gcc_checking_assert (info == IPA_NODE_REF (node)); > > > > > this line of code was actually necessary when adding nodes possibly > > > invalidated addresses of all summaries - like fast_function_summary > > > classes still do. So if we ever decide to use fast summaries we > > > need a test to remind us that info address must be obtained again. > > Ok. I'm not aware that, will keep the line as original. > > > > Thanks, > > Feng
Re: [PATCH V2] Generalized value pass-through for self-recursive function (ipa/pr93203)
Hi Tarmar, Since I could not reproduce this ICE with my local testing on Spec2017, would you share your config, or command line options extracted from compilation of perlbench? Thanks, Feng From: gcc-patches-ow...@gcc.gnu.org on behalf of Tamar Christina Sent: Tuesday, February 11, 2020 6:05 PM To: Feng Xue OS; Martin Jambor; Jan Hubicka; gcc-patches@gcc.gnu.org Cc: nd Subject: RE: [PATCH V2] Generalized value pass-through for self-recursive function (ipa/pr93203) Hi Feng, This patch (commit a0f6a8cb414b687f22c9011a894d5e8e398c4be0) is causing ICEs in the GCC and perlbench benchmark in Spec2017. during IPA pass: cp lto1: internal compiler error: in find_more_scalar_values_for_callers_subset, at ipa-cp.c:4709 0x1698187 find_more_scalar_values_for_callers_subset ../.././gcc/ipa-cp.c:4709 0x169f7d3 decide_about_value ../.././gcc/ipa-cp.c:5490 0x169fdc3 decide_whether_version_node ../.././gcc/ipa-cp.c:5537 0x169fdc3 ipcp_decision_stage ../.././gcc/ipa-cp.c:5718 0x169fdc3 ipcp_driver ../.././gcc/ipa-cp.c:5901 Please submit a full bug report, Thanks, Tamar > -Original Message- > From: gcc-patches-ow...@gcc.gnu.org > On Behalf Of Feng Xue OS > Sent: Monday, February 10, 2020 03:29 > To: Martin Jambor ; Jan Hubicka ; > gcc-patches@gcc.gnu.org > Subject: Re: [PATCH V2] Generalized value pass-through for self-recursive > function (ipa/pr93203) > > >> - gcc_checking_assert (item->value); > > > I've been staring at this for quite a while, trying to figure out how > > your patch can put NULL here before I realized it was just a clean-up > > :-) Sending such changes independently or pointing them out in the > > email/ChangeLog makes review easier. > > Ok. I'll add some description on this cleanup on ChangeLog. > > >> @@ -5564,7 +5610,6 @@ decide_whether_version_node (struct > cgraph_node *node) > >> } > >>clone = create_specialized_node (node, known_csts, known_contexts, > >> aggvals, callers); > >> - info = IPA_NODE_REF (node); > > > please either drop this change or change it to: > > > > gcc_checking_assert (info == IPA_NODE_REF (node)); > > > this line of code was actually necessary when adding nodes possibly > > invalidated addresses of all summaries - like fast_function_summary > > classes still do. So if we ever decide to use fast summaries we need > > a test to remind us that info address must be obtained again. > Ok. I'm not aware that, will keep the line as original. > > Thanks, > Feng
RE: [PATCH V2] Generalized value pass-through for self-recursive function (ipa/pr93203)
Hi Feng, This patch (commit a0f6a8cb414b687f22c9011a894d5e8e398c4be0) is causing ICEs in the GCC and perlbench benchmark in Spec2017. during IPA pass: cp lto1: internal compiler error: in find_more_scalar_values_for_callers_subset, at ipa-cp.c:4709 0x1698187 find_more_scalar_values_for_callers_subset ../.././gcc/ipa-cp.c:4709 0x169f7d3 decide_about_value ../.././gcc/ipa-cp.c:5490 0x169fdc3 decide_whether_version_node ../.././gcc/ipa-cp.c:5537 0x169fdc3 ipcp_decision_stage ../.././gcc/ipa-cp.c:5718 0x169fdc3 ipcp_driver ../.././gcc/ipa-cp.c:5901 Please submit a full bug report, Thanks, Tamar > -Original Message- > From: gcc-patches-ow...@gcc.gnu.org > On Behalf Of Feng Xue OS > Sent: Monday, February 10, 2020 03:29 > To: Martin Jambor ; Jan Hubicka ; > gcc-patches@gcc.gnu.org > Subject: Re: [PATCH V2] Generalized value pass-through for self-recursive > function (ipa/pr93203) > > >> - gcc_checking_assert (item->value); > > > I've been staring at this for quite a while, trying to figure out how > > your patch can put NULL here before I realized it was just a clean-up > > :-) Sending such changes independently or pointing them out in the > > email/ChangeLog makes review easier. > > Ok. I'll add some description on this cleanup on ChangeLog. > > >> @@ -5564,7 +5610,6 @@ decide_whether_version_node (struct > cgraph_node *node) > >> } > >>clone = create_specialized_node (node, known_csts, known_contexts, > >> aggvals, callers); > >> - info = IPA_NODE_REF (node); > > > please either drop this change or change it to: > > > > gcc_checking_assert (info == IPA_NODE_REF (node)); > > > this line of code was actually necessary when adding nodes possibly > > invalidated addresses of all summaries - like fast_function_summary > > classes still do. So if we ever decide to use fast summaries we need > > a test to remind us that info address must be obtained again. > Ok. I'm not aware that, will keep the line as original. > > Thanks, > Feng
Re: [PATCH V2] Generalized value pass-through for self-recursive function (ipa/pr93203)
>> - gcc_checking_assert (item->value); > I've been staring at this for quite a while, trying to figure out how > your patch can put NULL here before I realized it was just a clean-up > :-) Sending such changes independently or pointing them out in the > email/ChangeLog makes review easier. Ok. I'll add some description on this cleanup on ChangeLog. >> @@ -5564,7 +5610,6 @@ decide_whether_version_node (struct cgraph_node *node) >> } >>clone = create_specialized_node (node, known_csts, known_contexts, >> aggvals, callers); >> - info = IPA_NODE_REF (node); > please either drop this change or change it to: > > gcc_checking_assert (info == IPA_NODE_REF (node)); > this line of code was actually necessary when adding nodes possibly > invalidated addresses of all summaries - like fast_function_summary > classes still do. So if we ever decide to use fast summaries we need a > test to remind us that info address must be obtained again. Ok. I'm not aware that, will keep the line as original. Thanks, Feng
Re: [PATCH V2] Generalized value pass-through for self-recursive function (ipa/pr93203)
Hi,
On Sat, Jan 25 2020, Feng Xue OS wrote:
> Made some changes.
>
> Feng
>
>
> From: Feng Xue OS
> Sent: Saturday, January 25, 2020 5:54 PM
> To: mjam...@suse.cz; Jan Hubicka; gcc-patches@gcc.gnu.org
> Subject: [PATCH] Generalized value pass-through for self-recursive function
> (ipa/pr93203)
>
> Besides simple pass-through (aggregate) jump function, arithmetic (aggregate)
> jump function could also bring same (aggregate) value as parameter passed-in
> for self-feeding recursive call. For example,
>
> f1 (int i)/* normal jump function */
> {
> f1 (i & 1);
> }
>
> Suppose i is 0, recursive propagation via (i & 1) also gets 0, which
> can be seen as a simple pass-through of i.
>
> f2 (int *p) /* aggregate jump function */
> {
> int t = *p & 1;
> f2 ();
> }
> Likewise, if *p is 0, (*p & 1) is also 0, and is an aggregate simple
> pass-through of p.
>
> This patch is to support these two kinds of value pass-through.
> Bootstrapped/regtested on x86_64-linux and aarch64-linux.
sorry for the delay in the review. As far as I am concerned, I am OK
with the patch but please see the few comments below:
> ---
> 2020-01-25 Feng Xue
>
> PR ipa/93203
> * ipa-cp.c (ipcp_lattice::add_value): Add source with same call
> edge but different source value.
> (adjust_callers_for_value_intersection): New function.
> (gather_edges_for_value): Adjust order of callers to let a
> non-self-recursive caller be the first element.
> (self_recursive_pass_through_p): Add a new parameter simple, and
> check generalized self-recursive pass-through jump function.
> (self_recursive_agg_pass_through_p): Likewise.
> (find_more_scalar_values_for_callers_subset): Compute value from
> pass-through jump function for self-recursive.
> (intersect_with_plats): Remove code of itersection with unknown
> place holder value.
> (intersect_with_agg_replacements): Likewise.
> (intersect_aggregates_with_edge): Deduce with from pass-through
> jump function for self-recursive.
> (decide_whether_version_node): Remove dead callers and adjust
> order to let a non-self-recursive caller be the first element.
>
> From 74aef0cd2f40ff828a4b2abcbbdbbf4b1aea1fcf Mon Sep 17 00:00:00 2001
> From: Feng Xue
> Date: Tue, 21 Jan 2020 20:53:38 +0800
> Subject: [PATCH] Generalized value pass-through for self-recusive function
>
> ---
> gcc/ipa-cp.c | 195 ++---
> gcc/testsuite/g++.dg/ipa/pr93203.C | 95 ++
> gcc/testsuite/gcc.dg/ipa/ipcp-1.c | 2 +-
> 3 files changed, 216 insertions(+), 76 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/ipa/pr93203.C
>
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 17da1d8e8a7..64d23a34292 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
...
> @@ -4817,19 +4867,12 @@ intersect_with_plats (class ipcp_param_lattices
> *plats,
> break;
> if (aglat->offset - offset == item->offset)
> {
> - gcc_checking_assert (item->value);
I've been staring at this for quite a while, trying to figure out how
your patch can put NULL here before I realized it was just a clean-up
:-) Sending such changes independently or pointing them out in the
email/ChangeLog makes review easier.
> if (aglat->is_single_const ())
> {
> tree value = aglat->values->value;
>
> if (values_equal_for_ipcp_p (item->value, value))
> found = true;
> - else if (item->value == error_mark_node)
> - {
> - /* Replace unknown place holder value with real one. */
> - item->value = value;
> - found = true;
> - }
> }
> break;
> }
...
> @@ -5564,7 +5610,6 @@ decide_whether_version_node (struct cgraph_node *node)
> }
>clone = create_specialized_node (node, known_csts, known_contexts,
> aggvals, callers);
> - info = IPA_NODE_REF (node);
please either drop this change or change it to:
gcc_checking_assert (info == IPA_NODE_REF (node));
this line of code was actually necessary when adding nodes possibly
invalidated addresses of all summaries - like fast_function_summary
classes still do. So if we ever decide to use fast summaries we need a
test to remind us that info address must be obtained again.
Thanks for working on this!
Martin
