diff mbox series

Move array_bounds warnings into it's own pass.

Message ID 7972a25c-e7b2-4cc4-a6f0-e16719f452e6@redhat.com
State New
Headers show
Series Move array_bounds warnings into it's own pass. | expand

Commit Message

Andrew MacLeod June 10, 2024, 7:24 p.m. UTC 
The array bounds warning pass was originally attached to the VRP pass 
because it wanted to leverage the context sensitive ranges available there.

With ranger, we can make it a pass of its own for very little cost.  
This patch does that. It removes the array_bounds_checker from VRP and 
makes it a solo pass that runs immediately after VRP1.

The original version had VRP add any un-executable edge flags it found, 
but I could not find a case where after VRP cleans up the CFG the new 
pass needed that.  I also did not find a case where activating SCEV 
again for the warning pass made a difference after VRP had run.  So this 
patch does neither of those things.

It simple enough to later add SCEV and loop analysis again if it turns 
out to be important.

My primary motivation for removing it was to remove the second DOM walk 
the checker performs which depends on on-demand ranges pre-cached by  
ranger.   This prevented VRP from choosing an alternative VRP solution 
when basic block counts are very high (PR  114855).  I also know Siddesh 
want to experiment with moving the pass later in the pipeline as well, 
which will make that task much simpler as a secondary rationale.

I didn't want to mess with the internal code much. For a multitude of 
reasons.  I did change it so that it always uses the current range_query 
object instead of passing one in to the constructor.  And then I cleaned 
up the VRP code ot no longer take a flag on whether to invoke the 
warning code or not.

The final bit is the pass is set to only run when flag_tree_vrp is on..  
I did this primarily to preserve existing functionality, and some tests 
depended on it.  ie, would turn on -warray-bounds and disables tree-vrp 
pass (which means the  bounds checker doesnt run) ... which changes the 
expected warnings from the strlen pass.    I'm not going there.    there 
are  also tests which run at -O1 and -Wall that do not expect the bounds 
checker to run either.   So this dependence on the vrp flag is 
documented in the code an preserves existing behavior.

Does anyone have any issues with any of this?

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

Andrew

Comments

Jeff Law June 10, 2024, 8:12 p.m. UTC | #1 
On 6/10/24 1:24 PM, Andrew MacLeod wrote:
> The array bounds warning pass was originally attached to the VRP pass 
> because it wanted to leverage the context sensitive ranges available there.
> 
> With ranger, we can make it a pass of its own for very little cost. This 
> patch does that. It removes the array_bounds_checker from VRP and makes 
> it a solo pass that runs immediately after VRP1.
> 
> The original version had VRP add any un-executable edge flags it found, 
> but I could not find a case where after VRP cleans up the CFG the new 
> pass needed that.  I also did not find a case where activating SCEV 
> again for the warning pass made a difference after VRP had run.  So this 
> patch does neither of those things.
> 
> It simple enough to later add SCEV and loop analysis again if it turns 
> out to be important.
> 
> My primary motivation for removing it was to remove the second DOM walk 
> the checker performs which depends on on-demand ranges pre-cached by 
> ranger.   This prevented VRP from choosing an alternative VRP solution 
> when basic block counts are very high (PR  114855).  I also know Siddesh 
> want to experiment with moving the pass later in the pipeline as well, 
> which will make that task much simpler as a secondary rationale.
> 
> I didn't want to mess with the internal code much. For a multitude of 
> reasons.  I did change it so that it always uses the current range_query 
> object instead of passing one in to the constructor.  And then I cleaned 
> up the VRP code ot no longer take a flag on whether to invoke the 
> warning code or not.
> 
> The final bit is the pass is set to only run when flag_tree_vrp is on.. 
> I did this primarily to preserve existing functionality, and some tests 
> depended on it.  ie, would turn on -warray-bounds and disables tree-vrp 
> pass (which means the  bounds checker doesnt run) ... which changes the 
> expected warnings from the strlen pass.    I'm not going there.    there 
> are  also tests which run at -O1 and -Wall that do not expect the bounds 
> checker to run either.   So this dependence on the vrp flag is 
> documented in the code an preserves existing behavior.
> 
> Does anyone have any issues with any of this?
No, in fact, quite the opposite.  I think we very much want the warning 
out of VRP into its own little pass that we can put wherever it makes 
sense in the pipeline rather than having it be tied to VRP.

I'd probably look at the -O1 vs -Wall stuff independently so that we 
could (in theory) eventually remove the dependence on flag_vrp.

jeff
Andrew MacLeod June 10, 2024, 8:16 p.m. UTC | #2 
On 6/10/24 16:12, Jeff Law wrote:
>
>
>>
>> Does anyone have any issues with any of this?
> No, in fact, quite the opposite.  I think we very much want the 
> warning out of VRP into its own little pass that we can put wherever 
> it makes sense in the pipeline rather than having it be tied to VRP.
>
> I'd probably look at the -O1 vs -Wall stuff independently so that we 
> could (in theory) eventually remove the dependence on flag_vrp.
>
>
I figured as much about removing the dependence on flag_tree_vrp .  I'm 
just tried of poking at it :-)  It shouldn't be difficult.

I'll check it in now and see what fallout ensues.


Andrew
Andrew MacLeod June 10, 2024, 8:30 p.m. UTC | #3 
pushed as 74ee12ff68243bb177fb8653474dff80c3792139


fyi, the 2 testcases depending on the VRP flag were:

c-c++-common/Warray-bounds-2.c   (-warray-bounds  -fno-tree-vrp :-P)
and
g++.dg/warn/string1.C   (-O1 -Wall)

Andrew

On 6/10/24 16:12, Jeff Law wrote:
>
>
> On 6/10/24 1:24 PM, Andrew MacLeod wrote:
>> The array bounds warning pass was originally attached to the VRP pass 
>> because it wanted to leverage the context sensitive ranges available 
>> there.
>>
>> With ranger, we can make it a pass of its own for very little cost. 
>> This patch does that. It removes the array_bounds_checker from VRP 
>> and makes it a solo pass that runs immediately after VRP1.
>>
>> The original version had VRP add any un-executable edge flags it 
>> found, but I could not find a case where after VRP cleans up the CFG 
>> the new pass needed that.  I also did not find a case where 
>> activating SCEV again for the warning pass made a difference after 
>> VRP had run.  So this patch does neither of those things.
>>
>> It simple enough to later add SCEV and loop analysis again if it 
>> turns out to be important.
>>
>> My primary motivation for removing it was to remove the second DOM 
>> walk the checker performs which depends on on-demand ranges 
>> pre-cached by ranger.   This prevented VRP from choosing an 
>> alternative VRP solution when basic block counts are very high (PR  
>> 114855).  I also know Siddesh want to experiment with moving the pass 
>> later in the pipeline as well, which will make that task much simpler 
>> as a secondary rationale.
>>
>> I didn't want to mess with the internal code much. For a multitude of 
>> reasons.  I did change it so that it always uses the current 
>> range_query object instead of passing one in to the constructor.  And 
>> then I cleaned up the VRP code ot no longer take a flag on whether to 
>> invoke the warning code or not.
>>
>> The final bit is the pass is set to only run when flag_tree_vrp is 
>> on.. I did this primarily to preserve existing functionality, and 
>> some tests depended on it.  ie, would turn on -warray-bounds and 
>> disables tree-vrp pass (which means the  bounds checker doesnt run) 
>> ... which changes the expected warnings from the strlen pass.    I'm 
>> not going there.    there are  also tests which run at -O1 and -Wall 
>> that do not expect the bounds checker to run either.   So this 
>> dependence on the vrp flag is documented in the code an preserves 
>> existing behavior.
>>
>> Does anyone have any issues with any of this?
> No, in fact, quite the opposite.  I think we very much want the 
> warning out of VRP into its own little pass that we can put wherever 
> it makes sense in the pipeline rather than having it be tied to VRP.
>
> I'd probably look at the -O1 vs -Wall stuff independently so that we 
> could (in theory) eventually remove the dependence on flag_vrp.
>
> jeff
>
>
diff mbox series

Patch

From aa0259784b0c0884956a627b78c0f5025d76c931 Mon Sep 17 00:00:00 2001
From: Andrew MacLeod <amacleod@redhat.com>
Date: Wed, 5 Jun 2024 15:12:27 -0400
Subject: [PATCH 2/2] Move array_bounds warnings into it's own pass.

Array bounds checking is currently tied to VRP.  This causes issues with
using laternate VRP algorithms as well as experimenting with moving
the location of the warnings later.   This moves it to its own pass
and cleans up the vrp_pass object.

	* gimple-array-bounds.cc (array_bounds_checker::array_bounds_checker):
	Always use current range_query.
	(pass_data_array_bounds): New.
	(pass_array_bounds): New.
	(make_pass_array_bounds): New.
	* gimple-array-bounds.h  (array_bounds_checker): Adjust prototype.
	* timevar.def (TV_TREE_ARRAY_BOUNDS): New timevar.
	* tree-pass.h (make_pass_array_bounds): Add prototype.
	* tree-vrp.cc (execute_ranger_vrp): Remove warning param and do
	not invoke array bounds warning pass.
	(pass_vrp::pass_vrp): Adjust params.
	(pass_vrp::close): Adjust parameters.
	(pass_vrp::warn_array_bounds_p): Remove.
	(make_pass_vrp): Remove warning param.
	(make_pass_early_vrp): Remove warning param.
	(make_pass_fast_vrp): Remove warning param.
---
 gcc/gimple-array-bounds.cc | 63 +++++++++++++++++++++++++++++++++-----
 gcc/gimple-array-bounds.h  |  2 +-
 gcc/passes.def             |  1 +
 gcc/timevar.def            |  1 +
 gcc/tree-pass.h            |  1 +
 gcc/tree-vrp.cc            | 40 +++++-------------------
 6 files changed, 67 insertions(+), 41 deletions(-)

diff --git a/gcc/gimple-array-bounds.cc b/gcc/gimple-array-bounds.cc
index 008071cd546..1d14abf3ca1 100644
--- a/gcc/gimple-array-bounds.cc
+++ b/gcc/gimple-array-bounds.cc
@@ -38,10 +38,12 @@  along with GCC; see the file COPYING3.  If not see
 #include "domwalk.h"
 #include "tree-cfg.h"
 #include "attribs.h"
+#include "tree-pass.h"
+#include "gimple-range.h"
 
-array_bounds_checker::array_bounds_checker (struct function *func,
-					    range_query *qry)
-  : fun (func), m_ptr_qry (qry)
+// Always use the current range query for the bounds checker.
+array_bounds_checker::array_bounds_checker (struct function *func)
+  : fun (func), m_ptr_qry (get_range_query (func))
 {
   /* No-op.  */
 }
@@ -838,11 +840,7 @@  class check_array_bounds_dom_walker : public dom_walker
 {
 public:
   check_array_bounds_dom_walker (array_bounds_checker *checker)
-    : dom_walker (CDI_DOMINATORS,
-		  /* Discover non-executable edges, preserving EDGE_EXECUTABLE
-		     flags, so that we can merge in information on
-		     non-executable edges from vrp_folder .  */
-		  REACHABLE_BLOCKS_PRESERVING_FLAGS),
+    : dom_walker (CDI_DOMINATORS, REACHABLE_BLOCKS),
     checker (checker) { }
   ~check_array_bounds_dom_walker () {}
 
@@ -888,3 +886,52 @@  array_bounds_checker::check ()
   check_array_bounds_dom_walker w (this);
   w.walk (ENTRY_BLOCK_PTR_FOR_FN (fun));
 }
+
+const pass_data pass_data_array_bounds =
+{
+  GIMPLE_PASS, /* type */
+  "bounds", /* name */
+  OPTGROUP_NONE, /* optinfo_flags */
+  TV_TREE_ARRAY_BOUNDS, /* tv_id */
+  PROP_ssa, /* properties_required */
+  0, /* properties_provided */
+  0, /* properties_destroyed */
+  0, /* todo_flags_start */
+  ( 0 ),  /* No TODOs */
+};
+
+class pass_array_bounds : public gimple_opt_pass
+{
+public:
+  pass_array_bounds (gcc::context *ctxt, const pass_data &data_)
+    : gimple_opt_pass (data_, ctxt), data (data_)
+    { }
+
+  /* opt_pass methods: */
+  opt_pass * clone () final override
+    { return new pass_array_bounds (m_ctxt, data); }
+  bool gate (function *) final override
+    {
+      // Gate on the VRP pass to preserve previous behavior.
+      return flag_tree_vrp && (warn_array_bounds || warn_strict_flex_arrays);
+    }
+  unsigned int execute (function *fun) final override
+    {
+      calculate_dominance_info (CDI_DOMINATORS);
+      // Enable ranger as the current range query.
+      enable_ranger (fun, false);
+      array_bounds_checker array_checker (fun);
+      array_checker.check ();
+      disable_ranger (fun);
+      return 0;
+    }
+
+ private:
+  const pass_data &data;
+}; // class pass_array_bounds
+
+gimple_opt_pass *
+make_pass_array_bounds (gcc::context *ctxt)
+{
+  return new pass_array_bounds (ctxt, pass_data_array_bounds);
+}
diff --git a/gcc/gimple-array-bounds.h b/gcc/gimple-array-bounds.h
index 3e077d0178f..aa7ca8e9730 100644
--- a/gcc/gimple-array-bounds.h
+++ b/gcc/gimple-array-bounds.h
@@ -27,7 +27,7 @@  class array_bounds_checker
   friend class check_array_bounds_dom_walker;
 
 public:
-  array_bounds_checker (struct function *, range_query *);
+  array_bounds_checker (struct function *);
   void check ();
 
 private:
diff --git a/gcc/passes.def b/gcc/passes.def
index 1cbbd413097..041229e47a6 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -226,6 +226,7 @@  along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_merge_phi);
       NEXT_PASS (pass_thread_jumps_full, /*first=*/true);
       NEXT_PASS (pass_vrp, false /* final_p*/);
+      NEXT_PASS (pass_array_bounds);
       NEXT_PASS (pass_dse);
       NEXT_PASS (pass_dce);
       /* pass_stdarg is always run and at this point we execute
diff --git a/gcc/timevar.def b/gcc/timevar.def
index 8e2168e0817..6fc36859138 100644
--- a/gcc/timevar.def
+++ b/gcc/timevar.def
@@ -161,6 +161,7 @@  DEFTIMEVAR (TV_TREE_VRP              , "tree VRP")
 DEFTIMEVAR (TV_TREE_VRP_THREADER     , "tree VRP threader")
 DEFTIMEVAR (TV_TREE_EARLY_VRP        , "tree Early VRP")
 DEFTIMEVAR (TV_TREE_FAST_VRP         , "tree Fast VRP")
+DEFTIMEVAR (TV_TREE_ARRAY_BOUNDS     , "warn array bounds")
 DEFTIMEVAR (TV_TREE_COPY_PROP        , "tree copy propagation")
 DEFTIMEVAR (TV_FIND_REFERENCED_VARS  , "tree find ref. vars")
 DEFTIMEVAR (TV_TREE_PTA		     , "tree PTA")
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index 29267589eeb..edebb2be245 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -470,6 +470,7 @@  extern gimple_opt_pass *make_pass_fre (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_check_data_deps (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_copy_prop (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_isolate_erroneous_paths (gcc::context *ctxt);
+extern gimple_opt_pass *make_pass_array_bounds (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_early_vrp (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_fast_vrp (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_vrp (gcc::context *ctxt);
diff --git a/gcc/tree-vrp.cc b/gcc/tree-vrp.cc
index 1c7b451d8fb..1f6b578f253 100644
--- a/gcc/tree-vrp.cc
+++ b/gcc/tree-vrp.cc
@@ -1095,8 +1095,7 @@  private:
   from anywhere to perform a VRP pass, including from EVRP.  */
 
 unsigned int
-execute_ranger_vrp (struct function *fun, bool warn_array_bounds_p,
-		    bool final_p)
+execute_ranger_vrp (struct function *fun, bool final_p)
 {
   loop_optimizer_init (LOOPS_NORMAL | LOOPS_HAVE_RECORDED_EXITS);
   rewrite_into_loop_closed_ssa (NULL, TODO_update_ssa);
@@ -1113,27 +1112,6 @@  execute_ranger_vrp (struct function *fun, bool warn_array_bounds_p,
   if (dump_file && (dump_flags & TDF_DETAILS))
     ranger->dump (dump_file);
 
-  if ((warn_array_bounds || warn_strict_flex_arrays) && warn_array_bounds_p)
-    {
-      // Set all edges as executable, except those ranger says aren't.
-      int non_exec_flag = ranger->non_executable_edge_flag;
-      basic_block bb;
-      FOR_ALL_BB_FN (bb, fun)
-	{
-	  edge_iterator ei;
-	  edge e;
-	  FOR_EACH_EDGE (e, ei, bb->succs)
-	    if (e->flags & non_exec_flag)
-	      e->flags &= ~EDGE_EXECUTABLE;
-	    else
-	      e->flags |= EDGE_EXECUTABLE;
-	}
-      scev_reset ();
-      array_bounds_checker array_checker (fun, ranger);
-      array_checker.check ();
-    }
-
-
   if (Value_Range::supports_type_p (TREE_TYPE
 				     (TREE_TYPE (current_function_decl)))
       && flag_ipa_vrp
@@ -1330,14 +1308,13 @@  const pass_data pass_data_fast_vrp =
 class pass_vrp : public gimple_opt_pass
 {
 public:
-  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)
+  pass_vrp (gcc::context *ctxt, const pass_data &data_)
+    : gimple_opt_pass (data_, ctxt), data (data_), final_p (false)
     { }
 
   /* opt_pass methods: */
   opt_pass * clone () final override
-    { return new pass_vrp (m_ctxt, data, false); }
+    { return new pass_vrp (m_ctxt, data); }
   void set_pass_param (unsigned int n, bool param) final override
     {
       gcc_assert (n == 0);
@@ -1350,12 +1327,11 @@  public:
       if (&data == &pass_data_fast_vrp)
 	return execute_fast_vrp (fun);
 
-      return execute_ranger_vrp (fun, warn_array_bounds_p, final_p);
+      return execute_ranger_vrp (fun, final_p);
     }
 
  private:
   const pass_data &data;
-  bool warn_array_bounds_p;
   bool final_p;
 }; // class pass_vrp
 
@@ -1426,19 +1402,19 @@  public:
 gimple_opt_pass *
 make_pass_vrp (gcc::context *ctxt)
 {
-  return new pass_vrp (ctxt, pass_data_vrp, true);
+  return new pass_vrp (ctxt, pass_data_vrp);
 }
 
 gimple_opt_pass *
 make_pass_early_vrp (gcc::context *ctxt)
 {
-  return new pass_vrp (ctxt, pass_data_early_vrp, false);
+  return new pass_vrp (ctxt, pass_data_early_vrp);
 }
 
 gimple_opt_pass *
 make_pass_fast_vrp (gcc::context *ctxt)
 {
-  return new pass_vrp (ctxt, pass_data_fast_vrp, false);
+  return new pass_vrp (ctxt, pass_data_fast_vrp);
 }
 
 gimple_opt_pass *
-- 
2.45.0