[COMMITTED] Remove pass counting in VRP.

[COMMITTED] Remove pass counting in VRP.

[Andrew MacLeod]

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

Pass counting in VRP is used to decide when to call early VRP, pass the 
flag to enable warnings, and when the final pass is.

If you try to add additional passes, this becomes quite fragile. This 
patch simply chooses the pass based on the data pointer passed in, and 
remove the pass counter.   The first FULL VRP pass invokes the warning 
code, and the flag passed in now represents the FINAL pass of VRP.  
There is no longer a global flag which, as it turns out, wasn't working 
well with the JIT compiler, but when undetected.  (Thanks to dmalcolm 
for helping me sort out what was going on there)


Bootstraps  on x86_64-pc-linux-gnu with no regressions.   Pushed.

Andrew

[-- Attachment #2: 0002-Remove-pass-counting-in-VRP.patch --]
[-- Type: text/x-patch, Size: 3647 bytes --]

From 29abc475a360ad14d5f692945f2805fba1fdc679 Mon Sep 17 00:00:00 2001
From: Andrew MacLeod <amacleod@redhat.com>
Date: Thu, 28 Sep 2023 09:19:32 -0400
Subject: [PATCH 2/5] Remove pass counting in VRP.

Rather than using a pass count to decide which parameters are passed to
VRP, makemit explicit.

	* passes.def (pass_vrp): Use parameter for final pass flag..
	* tree-vrp.cc (vrp_pass_num): Remove.
	(run_warning_pass): New.
	(pass_vrp::my_pass): Remove.
	(pass_vrp::final_p): New.
	(pass_vrp::set_pass_param): Set final_p param.
	(pass_vrp::execute): Choose specific pass based on data pointer.
---
 gcc/passes.def  |  4 ++--
 gcc/tree-vrp.cc | 26 +++++++++++++++++---------
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/gcc/passes.def b/gcc/passes.def
index 4110a472914..2bafd60bbfb 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -221,7 +221,7 @@ along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_fre, true /* may_iterate */);
       NEXT_PASS (pass_merge_phi);
       NEXT_PASS (pass_thread_jumps_full, /*first=*/true);
-      NEXT_PASS (pass_vrp, true /* warn_array_bounds_p */);
+      NEXT_PASS (pass_vrp, false /* final_p*/);
       NEXT_PASS (pass_dse);
       NEXT_PASS (pass_dce);
       /* pass_stdarg is always run and at this point we execute
@@ -348,7 +348,7 @@ along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_dominator, false /* may_peel_loop_headers_p */);
       NEXT_PASS (pass_strlen);
       NEXT_PASS (pass_thread_jumps_full, /*first=*/false);
-      NEXT_PASS (pass_vrp, false /* warn_array_bounds_p */);
+      NEXT_PASS (pass_vrp, true /* final_p */);
       /* Run CCP to compute alignment and nonzero bits.  */
       NEXT_PASS (pass_ccp, true /* nonzero_p */);
       NEXT_PASS (pass_warn_restrict);
diff --git a/gcc/tree-vrp.cc b/gcc/tree-vrp.cc
index d7b194f5904..05266dfe34a 100644
--- a/gcc/tree-vrp.cc
+++ b/gcc/tree-vrp.cc
@@ -1120,36 +1120,44 @@ const pass_data pass_data_early_vrp =
   ( TODO_cleanup_cfg | TODO_update_ssa | TODO_verify_all ),
 };
 
-static int vrp_pass_num = 0;
+static bool run_warning_pass = true;
 class pass_vrp : public gimple_opt_pass
 {
 public:
   pass_vrp (gcc::context *ctxt, const pass_data &data_)
-    : gimple_opt_pass (data_, ctxt), data (data_), warn_array_bounds_p (false),
-      my_pass (vrp_pass_num++)
-  {}
+    : gimple_opt_pass (data_, ctxt), data (data_),
+      warn_array_bounds_p (false), final_p (false)
+  {
+    // Only the frst VRP pass should run warnings.
+    if (&data == &pass_data_vrp)
+      {
+	warn_array_bounds_p = run_warning_pass;
+	run_warning_pass = false;
+      }
+  }
 
   /* opt_pass methods: */
   opt_pass * clone () final override { return new pass_vrp (m_ctxt, data); }
   void set_pass_param (unsigned int n, bool param) final override
     {
       gcc_assert (n == 0);
-      warn_array_bounds_p = param;
+      final_p = param;
     }
   bool gate (function *) final override { return flag_tree_vrp != 0; }
   unsigned int execute (function *fun) final override
     {
       // Early VRP pass.
-      if (my_pass == 0)
-	return execute_ranger_vrp (fun, /*warn_array_bounds_p=*/false, false);
+      if (&data == &pass_data_early_vrp)
+	return execute_ranger_vrp (fun, /*warn_array_bounds_p=*/false,
+				   /*final_p=*/false);
 
-      return execute_ranger_vrp (fun, warn_array_bounds_p, my_pass == 2);
+      return execute_ranger_vrp (fun, warn_array_bounds_p, final_p);
     }
 
  private:
   const pass_data &data;
   bool warn_array_bounds_p;
-  int my_pass;
+  bool final_p;
 }; // class pass_vrp
 
 const pass_data pass_data_assumptions =
-- 
2.41.0

Re: [COMMITTED] Remove pass counting in VRP.

[David Malcolm]

On Tue, 2023-10-03 at 10:32 -0400, Andrew MacLeod wrote:
> Pass counting in VRP is used to decide when to call early VRP, pass
> the 
> flag to enable warnings, and when the final pass is.
> 
> If you try to add additional passes, this becomes quite fragile. This
> patch simply chooses the pass based on the data pointer passed in,
> and 
> remove the pass counter.   The first FULL VRP pass invokes the
> warning 
> code, and the flag passed in now represents the FINAL pass of VRP.  
> There is no longer a global flag which, as it turns out, wasn't
> working 
> well with the JIT compiler, but when undetected.  (Thanks to dmalcolm
> for helping me sort out what was going on there)
> 
> 
> Bootstraps  on x86_64-pc-linux-gnu with no regressions.   Pushed.

[CCing jit mailing list]

I'm worried that this patch may have "papered over" an issue with
libgccjit.  Specifically:

[...snip...]

> diff --git a/gcc/tree-vrp.cc b/gcc/tree-vrp.cc
> index d7b194f5904..05266dfe34a 100644
> --- a/gcc/tree-vrp.cc
> +++ b/gcc/tree-vrp.cc
> @@ -1120,36 +1120,44 @@ const pass_data pass_data_early_vrp =
>    ( TODO_cleanup_cfg | TODO_update_ssa | TODO_verify_all ),
>  };
>  
> -static int vrp_pass_num = 0;
> +static bool run_warning_pass = true;

I see the global variable "run_warning_pass" starts out true here

>  class pass_vrp : public gimple_opt_pass
>  {
>  public:
>    pass_vrp (gcc::context *ctxt, const pass_data &data_)
> -    : gimple_opt_pass (data_, ctxt), data (data_), warn_array_bounds_p (false),
> -      my_pass (vrp_pass_num++)
> -  {}
> +    : gimple_opt_pass (data_, ctxt), data (data_),
> +      warn_array_bounds_p (false), final_p (false)
> +  {
> +    // Only the frst VRP pass should run warnings.
> +    if (&data == &pass_data_vrp)
> +      {
> +	warn_array_bounds_p = run_warning_pass;
> +	run_warning_pass = false;

...and run_warning_pass affects the member data
pass_vrp::warn_array_bounds_p here, and then becomes false, but nothing
seems to ever reset run_warning_pass back to true.

It seems that with this patch, if libgccjit compiles more than one
gcc_jit_context in the same process, the first context compilation will
warn, whereas subsequent ones in that process won't.

Or did I miss something?

[...snip...]

Thoughts?
Dave

Re: [COMMITTED] Remove pass counting in VRP.

[Andrew MacLeod]

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


On 10/3/23 13:02, David Malcolm wrote:
> On Tue, 2023-10-03 at 10:32 -0400, Andrew MacLeod wrote:
>> Pass counting in VRP is used to decide when to call early VRP, pass
>> the
>> flag to enable warnings, and when the final pass is.
>>
>> If you try to add additional passes, this becomes quite fragile. This
>> patch simply chooses the pass based on the data pointer passed in,
>> and
>> remove the pass counter.   The first FULL VRP pass invokes the
>> warning
>> code, and the flag passed in now represents the FINAL pass of VRP.
>> There is no longer a global flag which, as it turns out, wasn't
>> working
>> well with the JIT compiler, but when undetected.  (Thanks to dmalcolm
>> for helping me sort out what was going on there)
>>
>>
>> Bootstraps  on x86_64-pc-linux-gnu with no regressions.   Pushed.
> [CCing jit mailing list]
>
> I'm worried that this patch may have "papered over" an issue with
> libgccjit.  Specifically:

well, that isnt the patch that was checked in :-P

Im not sure how the old version got into the commit note.

Attached is the version checked in.


[-- Attachment #2: pass.diff --]
[-- Type: text/x-patch, Size: 3887 bytes --]

commit 7eb5ce7f58ed4a48641e1786e4fdeb2f7fb8c5ff
Author: Andrew MacLeod <amacleod@redhat.com>
Date:   Thu Sep 28 09:19:32 2023 -0400

    Remove pass counting in VRP.
    
    Rather than using a pass count to decide which parameters are passed to
    VRP, makemit explicit.
    
            * passes.def (pass_vrp): Pass "final pass" flag as parameter.
            * tree-vrp.cc (vrp_pass_num): Remove.
            (pass_vrp::my_pass): Remove.
            (pass_vrp::pass_vrp): Add warn_p as a parameter.
            (pass_vrp::final_p): New.
            (pass_vrp::set_pass_param): Set final_p param.
            (pass_vrp::execute): Call execute_range_vrp with no conditions.
            (make_pass_vrp): Pass additional parameter.
            (make_pass_early_vrp): Ditto.

diff --git a/gcc/passes.def b/gcc/passes.def
index 4110a472914..2bafd60bbfb 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -221,7 +221,7 @@ along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_fre, true /* may_iterate */);
       NEXT_PASS (pass_merge_phi);
       NEXT_PASS (pass_thread_jumps_full, /*first=*/true);
-      NEXT_PASS (pass_vrp, true /* warn_array_bounds_p */);
+      NEXT_PASS (pass_vrp, false /* final_p*/);
       NEXT_PASS (pass_dse);
       NEXT_PASS (pass_dce);
       /* pass_stdarg is always run and at this point we execute
@@ -348,7 +348,7 @@ along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_dominator, false /* may_peel_loop_headers_p */);
       NEXT_PASS (pass_strlen);
       NEXT_PASS (pass_thread_jumps_full, /*first=*/false);
-      NEXT_PASS (pass_vrp, false /* warn_array_bounds_p */);
+      NEXT_PASS (pass_vrp, true /* final_p */);
       /* Run CCP to compute alignment and nonzero bits.  */
       NEXT_PASS (pass_ccp, true /* nonzero_p */);
       NEXT_PASS (pass_warn_restrict);
diff --git a/gcc/tree-vrp.cc b/gcc/tree-vrp.cc
index d7b194f5904..4f8c7745461 100644
--- a/gcc/tree-vrp.cc
+++ b/gcc/tree-vrp.cc
@@ -1120,36 +1120,32 @@ const pass_data pass_data_early_vrp =
   ( TODO_cleanup_cfg | TODO_update_ssa | TODO_verify_all ),
 };
 
-static int vrp_pass_num = 0;
 class pass_vrp : public gimple_opt_pass
 {
 public:
-  pass_vrp (gcc::context *ctxt, const pass_data &data_)
-    : gimple_opt_pass (data_, ctxt), data (data_), warn_array_bounds_p (false),
-      my_pass (vrp_pass_num++)
-  {}
+  pass_vrp (gcc::context *ctxt, const pass_data &data_, bool warn_p)
+    : gimple_opt_pass (data_, ctxt), data (data_),
+      warn_array_bounds_p (warn_p), final_p (false)
+    { }
 
   /* opt_pass methods: */
-  opt_pass * clone () final override { return new pass_vrp (m_ctxt, data); }
+  opt_pass * clone () final override
+    { return new pass_vrp (m_ctxt, data, false); }
   void set_pass_param (unsigned int n, bool param) final override
     {
       gcc_assert (n == 0);
-      warn_array_bounds_p = param;
+      final_p = param;
     }
   bool gate (function *) final override { return flag_tree_vrp != 0; }
   unsigned int execute (function *fun) final override
     {
-      // Early VRP pass.
-      if (my_pass == 0)
-	return execute_ranger_vrp (fun, /*warn_array_bounds_p=*/false, false);
-
-      return execute_ranger_vrp (fun, warn_array_bounds_p, my_pass == 2);
+      return execute_ranger_vrp (fun, warn_array_bounds_p, final_p);
     }
 
  private:
   const pass_data &data;
   bool warn_array_bounds_p;
-  int my_pass;
+  bool final_p;
 }; // class pass_vrp
 
 const pass_data pass_data_assumptions =
@@ -1219,13 +1215,13 @@ public:
 gimple_opt_pass *
 make_pass_vrp (gcc::context *ctxt)
 {
-  return new pass_vrp (ctxt, pass_data_vrp);
+  return new pass_vrp (ctxt, pass_data_vrp, true);
 }
 
 gimple_opt_pass *
 make_pass_early_vrp (gcc::context *ctxt)
 {
-  return new pass_vrp (ctxt, pass_data_early_vrp);
+  return new pass_vrp (ctxt, pass_data_early_vrp, false);
 }
 
 gimple_opt_pass *

Re: [COMMITTED] Remove pass counting in VRP.

[David Malcolm]

On Tue, 2023-10-03 at 13:11 -0400, Andrew MacLeod wrote:
> 
> On 10/3/23 13:02, David Malcolm wrote:
> > On Tue, 2023-10-03 at 10:32 -0400, Andrew MacLeod wrote:
> > > Pass counting in VRP is used to decide when to call early VRP,
> > > pass
> > > the
> > > flag to enable warnings, and when the final pass is.
> > > 
> > > If you try to add additional passes, this becomes quite fragile.
> > > This
> > > patch simply chooses the pass based on the data pointer passed
> > > in,
> > > and
> > > remove the pass counter.   The first FULL VRP pass invokes the
> > > warning
> > > code, and the flag passed in now represents the FINAL pass of
> > > VRP.
> > > There is no longer a global flag which, as it turns out, wasn't
> > > working
> > > well with the JIT compiler, but when undetected.  (Thanks to
> > > dmalcolm
> > > for helping me sort out what was going on there)
> > > 
> > > 
> > > Bootstraps  on x86_64-pc-linux-gnu with no regressions.   Pushed.
> > [CCing jit mailing list]
> > 
> > I'm worried that this patch may have "papered over" an issue with
> > libgccjit.  Specifically:
> 
> well, that isnt the patch that was checked in :-P

Aha!  That makes much more sense.  I took a look at
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=7eb5ce7f58ed4a48641e1786e4fdeb2f7fb8c5ff
and yes, that looks like it will work with libgccjit

Thanks for clarifying (and for fixing the issue)
Dave

> 
> Im not sure how the old version got into the commit note.
> 
> Attached is the version checked in.
>