public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] ipa-cp: Do not create clones for values outside known value range (PR 102513)
@ 2022-02-14 18:17 Martin Jambor
  2022-03-23 15:03 ` [PATCH PING] " Martin Jambor
  2022-03-31 12:40 ` [PATCH] " Jan Hubicka
  0 siblings, 2 replies; 3+ messages in thread
From: Martin Jambor @ 2022-02-14 18:17 UTC (permalink / raw)
  To: GCC Patches; +Cc: Jan Hubicka

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)?

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH PING] ipa-cp: Do not create clones for values outside known value range (PR 102513)
  2022-02-14 18:17 [PATCH] ipa-cp: Do not create clones for values outside known value range (PR 102513) Martin Jambor
@ 2022-03-23 15:03 ` Martin Jambor
  2022-03-31 12:40 ` [PATCH] " Jan Hubicka
  1 sibling, 0 replies; 3+ messages in thread
From: Martin Jambor @ 2022-03-23 15:03 UTC (permalink / raw)
  To: GCC Patches; +Cc: Jan Hubicka, Jan Hubicka

Hello,

I would like to ping this patch, please.

Thanks,

Martin


On Mon, Feb 14 2022, Martin Jambor wrote:
> 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)?
>
> 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] ipa-cp: Do not create clones for values outside known value range (PR 102513)
  2022-02-14 18:17 [PATCH] ipa-cp: Do not create clones for values outside known value range (PR 102513) Martin Jambor
  2022-03-23 15:03 ` [PATCH PING] " Martin Jambor
@ 2022-03-31 12:40 ` Jan Hubicka
  1 sibling, 0 replies; 3+ messages in thread
From: Jan Hubicka @ 2022-03-31 12:40 UTC (permalink / raw)
  To: Martin Jambor; +Cc: GCC Patches

> 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
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-03-31 12:40 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-14 18:17 [PATCH] ipa-cp: Do not create clones for values outside known value range (PR 102513) Martin Jambor
2022-03-23 15:03 ` [PATCH PING] " Martin Jambor
2022-03-31 12:40 ` [PATCH] " Jan Hubicka

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).