[PATCH] PR tree-optimization/107570 - Reset SCEV after folding in VRP.

[PATCH] PR tree-optimization/107570 - Reset SCEV after folding in VRP.

[Andrew MacLeod]

[-- Attachment #1: Type: text/plain, Size: 380 bytes --]

We can reset SCEV after we fold, then SCEVs cache shouldn't have 
anything in it when we go to remove ssa-names in remove_unreachable().

We were resetting it later sometimes if we were processing the array 
bounds warning, so I removed that call and just always reset it now.

Bootstraps on x86_64-pc-linux-gnu. Testing running. Assuming no 
regressions,  OK for trunk?

Andrew

[-- Attachment #2: 0001-Reset-SCEV-after-folding-in-VRP.patch --]
[-- Type: text/x-patch, Size: 2259 bytes --]

From 450b058445ffb0a1ffbdec08732d4267f03a8ce5 Mon Sep 17 00:00:00 2001
From: Andrew MacLeod <amacleod@redhat.com>
Date: Wed, 1 Feb 2023 11:46:18 -0500
Subject: [PATCH] Reset SCEV after folding in VRP.

SCEV needs to be reset to processing array bounds in VRP, but it should
also be reset before trying to remove unreachable globals so it's cache
doesn't cause issues.

	PR tree-optimization/107570
	gcc/
	* tree-vrp.cc (execute_ranger_vrp): Reset SCEV after folding.

	gcc/testsuite/
	gcc.dg/pr107570.c: New.
---
 gcc/testsuite/gcc.dg/pr107570.c | 25 +++++++++++++++++++++++++
 gcc/tree-vrp.cc                 |  6 +++++-
 2 files changed, 30 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr107570.c

diff --git a/gcc/testsuite/gcc.dg/pr107570.c b/gcc/testsuite/gcc.dg/pr107570.c
new file mode 100644
index 00000000000..ba5b535a867
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr107570.c
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-options "-Os" } */
+
+long int n;
+
+void
+foo (int *p, int x)
+{
+  for (;;)
+    {
+      for (*p = 0; *p < 1; ++*p)
+        {
+          n += *p < 0;
+          if (n < x)
+            {
+              while (x < 1)
+                ++x;
+
+              __builtin_unreachable ();
+            }
+        }
+
+      p = &x;
+    }
+}
diff --git a/gcc/tree-vrp.cc b/gcc/tree-vrp.cc
index 3c431760a16..0b69374adba 100644
--- a/gcc/tree-vrp.cc
+++ b/gcc/tree-vrp.cc
@@ -1096,6 +1096,11 @@ execute_ranger_vrp (struct function *fun, bool warn_array_bounds_p,
   gimple_ranger *ranger = enable_ranger (fun, false);
   rvrp_folder folder (ranger);
   folder.substitute_and_fold ();
+
+  // SCEV needs to be reset for array bounds, and we do not wish to trigger
+  // any SCEV lookups when removing unreachable globals, so reset it here.
+  scev_reset ();
+
   // Remove tagged builtin-unreachable and maybe update globals.
   folder.m_unreachable.remove_and_update_globals (final_p);
   if (dump_file && (dump_flags & TDF_DETAILS))
@@ -1116,7 +1121,6 @@ execute_ranger_vrp (struct function *fun, bool warn_array_bounds_p,
 	    else
 	      e->flags |= EDGE_EXECUTABLE;
 	}
-      scev_reset ();
       array_bounds_checker array_checker (fun, ranger);
       array_checker.check ();
     }
-- 
2.39.0

Re: [PATCH] PR tree-optimization/107570 - Reset SCEV after folding in VRP.

[Richard Biener]

On Wed, Feb 1, 2023 at 7:12 PM Andrew MacLeod via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> We can reset SCEV after we fold, then SCEVs cache shouldn't have
> anything in it when we go to remove ssa-names in remove_unreachable().
>
> We were resetting it later sometimes if we were processing the array
> bounds warning, so I removed that call and just always reset it now.
>
> Bootstraps on x86_64-pc-linux-gnu. Testing running. Assuming no
> regressions,  OK for trunk?

+
+  // SCEV needs to be reset for array bounds, and we do not wish to trigger
+  // any SCEV lookups when removing unreachable globals, so reset it here.
+  scev_reset ();

the comment suggests that SCEV queries (aka analyze_scalar_evolution)
won't return anything after a scev_reset ().  That's not true - instead what
it does is nuke the SCEV cache.  That's necessary when you
release SSA names or alter the CFG and you want to avoid followup
SCEV queries to pick up stale data.

So if remove_and_update_globals performs SCEV queries and eventually
releases SSA names you cannot remove the second call to scev_reset.

But yes, it's probably substitute_and_fold_engine::substitute_and_fold
itself that should do a

  if (scev_initialized_p ())
    scev_reset ();

possibly only in the case it released an SSA name, or removed an
edge (but that's maybe premature optimization).

Richard.

>
> Andrew

Re: [PATCH] PR tree-optimization/107570 - Reset SCEV after folding in VRP.

[Andrew MacLeod]

[-- Attachment #1: Type: text/plain, Size: 1728 bytes --]


On 2/2/23 07:22, Richard Biener wrote:
> On Wed, Feb 1, 2023 at 7:12 PM Andrew MacLeod via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>> We can reset SCEV after we fold, then SCEVs cache shouldn't have
>> anything in it when we go to remove ssa-names in remove_unreachable().
>>
>> We were resetting it later sometimes if we were processing the array
>> bounds warning, so I removed that call and just always reset it now.
>>
>> Bootstraps on x86_64-pc-linux-gnu. Testing running. Assuming no
>> regressions,  OK for trunk?
> +
> +  // SCEV needs to be reset for array bounds, and we do not wish to trigger
> +  // any SCEV lookups when removing unreachable globals, so reset it here.
> +  scev_reset ();
>
> the comment suggests that SCEV queries (aka analyze_scalar_evolution)
> won't return anything after a scev_reset ().  That's not true - instead what
> it does is nuke the SCEV cache.  That's necessary when you
> release SSA names or alter the CFG and you want to avoid followup
> SCEV queries to pick up stale data.
>
> So if remove_and_update_globals performs SCEV queries and eventually
> releases SSA names you cannot remove the second call to scev_reset.
>
> But yes, it's probably substitute_and_fold_engine::substitute_and_fold
> itself that should do a
>
>    if (scev_initialized_p ())
>      scev_reset ();
>
> possibly only in the case it released an SSA name, or removed an
> edge (but that's maybe premature optimization).
>
> Richard.
>
>
Mmm, yeah thats not what I meant to imply.  We can simply reset it 
before trying to process the unreachable globals like so. Maybe we visit 
the  S&F engine next release.. It seems more intrusive than this.

How about this patch?  testing underway.

Andrew


[-- Attachment #2: 0001-Reset-SCEV-before-removing-unreachable-globals.patch --]
[-- Type: text/x-patch, Size: 1680 bytes --]

From e58ebd111e7b3b8047fb93396c204bd703926802 Mon Sep 17 00:00:00 2001
From: Andrew MacLeod <amacleod@redhat.com>
Date: Wed, 1 Feb 2023 11:46:18 -0500
Subject: [PATCH] Reset SCEV before removing unreachable globals.

SCEV should be reset in VRP before trying to remove unreachable globals
to avoid triggering issues with it's cache.

	PR tree-optimization/107570
	gcc/
	* tree-vrp.cc (remove_and_update_globals): Reset SCEV.

	gcc/testsuite/
	gcc.dg/pr107570.c: New.
---
 gcc/testsuite/gcc.dg/pr107570.c | 25 +++++++++++++++++++++++++
 gcc/tree-vrp.cc                 |  4 ++++
 2 files changed, 29 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/pr107570.c

diff --git a/gcc/testsuite/gcc.dg/pr107570.c b/gcc/testsuite/gcc.dg/pr107570.c
new file mode 100644
index 00000000000..ba5b535a867
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr107570.c
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-options "-Os" } */
+
+long int n;
+
+void
+foo (int *p, int x)
+{
+  for (;;)
+    {
+      for (*p = 0; *p < 1; ++*p)
+        {
+          n += *p < 0;
+          if (n < x)
+            {
+              while (x < 1)
+                ++x;
+
+              __builtin_unreachable ();
+            }
+        }
+
+      p = &x;
+    }
+}
diff --git a/gcc/tree-vrp.cc b/gcc/tree-vrp.cc
index 3c431760a16..95547e5419b 100644
--- a/gcc/tree-vrp.cc
+++ b/gcc/tree-vrp.cc
@@ -121,6 +121,10 @@ remove_unreachable::remove_and_update_globals (bool final_p)
   if (m_list.length () == 0)
     return false;
 
+  // Ensure the cache in SCEV has been cleared before processing
+  // globals to be removed.
+  scev_reset ();
+
   bool change = false;
   tree name;
   unsigned i;
-- 
2.39.0

Re: [PATCH] PR tree-optimization/107570 - Reset SCEV after folding in VRP.

[Richard Biener]

> Am 03.02.2023 um 16:54 schrieb Andrew MacLeod <amacleod@redhat.com>:
> 
> 
>> On 2/2/23 07:22, Richard Biener wrote:
>>> On Wed, Feb 1, 2023 at 7:12 PM Andrew MacLeod via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>> We can reset SCEV after we fold, then SCEVs cache shouldn't have
>>> anything in it when we go to remove ssa-names in remove_unreachable().
>>> 
>>> We were resetting it later sometimes if we were processing the array
>>> bounds warning, so I removed that call and just always reset it now.
>>> 
>>> Bootstraps on x86_64-pc-linux-gnu. Testing running. Assuming no
>>> regressions,  OK for trunk?
>> +
>> +  // SCEV needs to be reset for array bounds, and we do not wish to trigger
>> +  // any SCEV lookups when removing unreachable globals, so reset it here.
>> +  scev_reset ();
>> 
>> the comment suggests that SCEV queries (aka analyze_scalar_evolution)
>> won't return anything after a scev_reset ().  That's not true - instead what
>> it does is nuke the SCEV cache.  That's necessary when you
>> release SSA names or alter the CFG and you want to avoid followup
>> SCEV queries to pick up stale data.
>> 
>> So if remove_and_update_globals performs SCEV queries and eventually
>> releases SSA names you cannot remove the second call to scev_reset.
>> 
>> But yes, it's probably substitute_and_fold_engine::substitute_and_fold
>> itself that should do a
>> 
>>   if (scev_initialized_p ())
>>     scev_reset ();
>> 
>> possibly only in the case it released an SSA name, or removed an
>> edge (but that's maybe premature optimization).
>> 
>> Richard.
>> 
>> 
> Mmm, yeah thats not what I meant to imply.  We can simply reset it before trying to process the unreachable globals like so. Maybe we visit the  S&F engine next release.. It seems more intrusive than this.
> 
> How about this patch?  testing underway.

Lgtm

Richard 
> Andrew
> 
> <0001-Reset-SCEV-before-removing-unreachable-globals.patch>