| Message ID | 248800ec-0e0e-6cae-5aaa-a9c69cd5f46a@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [COMMITTED] Remove pass counting in VRP. | expand |
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
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.
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. >
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