public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jan Hubicka <hubicka@kam.mff.cuni.cz>
To: Martin Jambor <mjambor@suse.cz>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] ipa-cp: Do not create clones for values outside known value range (PR 102513)
Date: Thu, 31 Mar 2022 14:40:48 +0200	[thread overview]
Message-ID: <YkWhUHxoiqtyhfrX@kam.mff.cuni.cz> (raw)
In-Reply-To: <ri6wnhxqpmi.fsf@suse.cz>

> Hi,
> 
> PR 102513 shows we emit bogus array access warnings when IPA-CP
> creates clones specialized for values which it deduces from arithmetic
> jump functions describing self-recursive calls.  Those can however be
> avoided if we consult the IPA-VR information that the same pass also
> has.
> 
> The patch below does that at the stage when normally values are only
> examined for profitability.  It would be better not to create lattices
> describing such bogus values in the first place, however that presents
> an ordering problem, the pass currently propagates all information,
> and so both constants and VR, in no particular order when processing
> SCCs, and so this approach seemed much simpler.
> 
> I plan to rearrange the pass so that it clones in multiple passes over
> the call graph (or rather the lattice dependence graph) and it feels
> natural to only do propagation for these kinds of recursion in the
> second or later passes, which would fix the issue more elegantly.
> 
> Bootstrapped and tested on x86_64, OK for trunk (and perhaps also for
> GCC 11)?

Looks OK.  I wonder if we can do that more genrally to avoid inlining in
such cases as well?

Honza
> 
> Thanks,
> 
> Martin
> 
> 
> gcc/ChangeLog:
> 
> 2022-02-14  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR ipa/102513
> 	* ipa-cp.cc (decide_whether_version_node): Skip scalar values
> 	which do not fit the known value_range.
> 
> gcc/testsuite/ChangeLog:
> 
> 2022-02-14  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR ipa/102513
> 	* gcc.dg/ipa/pr102513.c: New test.
> ---
>  gcc/ipa-cp.cc                       | 28 ++++++++++++++++++++++--
>  gcc/testsuite/gcc.dg/ipa/pr102513.c | 33 +++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/ipa/pr102513.c
> 
> diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc
> index 453e9c93cc3..ec37632d487 100644
> --- a/gcc/ipa-cp.cc
> +++ b/gcc/ipa-cp.cc
> @@ -6154,8 +6154,32 @@ decide_whether_version_node (struct cgraph_node *node)
>  	{
>  	  ipcp_value<tree> *val;
>  	  for (val = lat->values; val; val = val->next)
> -	    ret |= decide_about_value (node, i, -1, val, &avals,
> -				       &self_gen_clones);
> +	    {
> +	      /* If some values generated for self-recursive calls with
> +		 arithmetic jump functions fall outside of the known
> +		 value_range for the parameter, we can skip them.  VR interface
> +		 supports this only for integers now.  */
> +	      if (TREE_CODE (val->value) == INTEGER_CST
> +		  && !plats->m_value_range.bottom_p ()
> +		  && !plats->m_value_range.m_vr.contains_p (val->value))
> +		{
> +		  /* This can happen also if a constant present in the source
> +		     code falls outside of the range of parameter's type, so we
> +		     cannot assert.  */
> +		  if (dump_file && (dump_flags & TDF_DETAILS))
> +		    {
> +		      fprintf (dump_file, " - skipping%s value ",
> +			       val->self_recursion_generated_p ()
> +			       ? " self_recursion_generated" : "");
> +		      print_ipcp_constant_value (dump_file, val->value);
> +		      fprintf (dump_file, " because it is outside known "
> +			       "value range.\n");
> +		    }
> +		  continue;
> +		}
> +	      ret |= decide_about_value (node, i, -1, val, &avals,
> +					 &self_gen_clones);
> +	    }
>  	}
>  
>        if (!plats->aggs_bottom)
> diff --git a/gcc/testsuite/gcc.dg/ipa/pr102513.c b/gcc/testsuite/gcc.dg/ipa/pr102513.c
> new file mode 100644
> index 00000000000..9ee5431b730
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/pr102513.c
> @@ -0,0 +1,33 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -Warray-bounds" } */
> +
> +extern int block2[7][256];
> +
> +static int encode_block(int block2[7][256], unsigned level)
> +{
> +    int best_score = 0;
> +
> +    for (unsigned x = 0; x < level; x++) {
> +        int v = block2[1][x];
> +        block2[level][x] = 0;
> +        best_score += v * v;
> +    }
> +
> +    if (level > 0 && best_score > 64) {
> +        int score = 0;
> +
> +        score += encode_block(block2, level - 1);
> +        score += encode_block(block2, level - 1);
> +
> +        if (score < best_score) {
> +            best_score = score;
> +        }
> +    }
> +
> +    return best_score;
> +}
> +
> +int foo(void)
> +{
> +    return encode_block(block2, 5);
> +}
> -- 
> 2.34.1
> 

      parent reply	other threads:[~2022-03-31 12:40 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-14 18:17 Martin Jambor
2022-03-23 15:03 ` [PATCH PING] " Martin Jambor
2022-03-31 12:40 ` Jan Hubicka [this message]

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=YkWhUHxoiqtyhfrX@kam.mff.cuni.cz \
    --to=hubicka@kam.mff.cuni.cz \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=mjambor@suse.cz \
    /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).