[PATCH] tree-optimization/112843 - update_stmt doing wrong things

[PATCH] tree-optimization/112843 - update_stmt doing wrong things

[Richard Biener]

The following removes range_query::update_stmt and its single
invocation from update_stmt_operands.  That function is not
supposed to look beyond the raw stmt contents of the passed
stmt since there's no guarantee about the rest of the IL.

I've successfully bootstrapped & tested the update_stmt_operands
hunk, now testing removal of the actual routine as well.  The
testcase that was added when introducing range_query::update_stmt
still passes.

OK to remove the implementation?  I don't see any way around
removing the call though.

Thanks,
Richard.

	PR tree-optimization/112843
	* tree-ssa-operands.cc (update_stmt_operands): Do not call
	update_stmt from ranger.
	* value-query.h (range_query::update_stmt): Remove.
	* gimple-range.h (gimple_ranger::update_stmt): Likewise.
	* gimple-range.cc (gimple_ranger::update_stmt): Likewise.
---
 gcc/gimple-range.cc      | 34 ----------------------------------
 gcc/gimple-range.h       |  1 -
 gcc/tree-ssa-operands.cc |  3 ---
 gcc/value-query.h        |  3 ---
 4 files changed, 41 deletions(-)

diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index 5e9bb397a20..84d2c7516e6 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -544,40 +544,6 @@ gimple_ranger::register_transitive_inferred_ranges (basic_block bb)
     }
 }
 
-// When a statement S has changed since the result was cached, re-evaluate
-// and update the global cache.
-
-void
-gimple_ranger::update_stmt (gimple *s)
-{
-  tree lhs = gimple_get_lhs (s);
-  if (!lhs || !gimple_range_ssa_p (lhs))
-    return;
-  Value_Range r (TREE_TYPE (lhs));
-  // Only update if it already had a value.
-  if (m_cache.get_global_range (r, lhs))
-    {
-      // Re-calculate a new value using just cache values.
-      Value_Range tmp (TREE_TYPE (lhs));
-      fold_using_range f;
-      fur_stmt src (s, &m_cache);
-      f.fold_stmt (tmp, s, src, lhs);
-
-      // Combine the new value with the old value to check for a change.
-      if (r.intersect (tmp))
-	{
-	  if (dump_file && (dump_flags & TDF_DETAILS))
-	    {
-	      print_generic_expr (dump_file, lhs, TDF_SLIM);
-	      fprintf (dump_file, " : global value re-evaluated to ");
-	      r.dump (dump_file);
-	      fputc ('\n', dump_file);
-	    }
-	  m_cache.set_global_range (lhs, r);
-	}
-    }
-}
-
 // This routine will export whatever global ranges are known to GCC
 // SSA_RANGE_NAME_INFO and SSA_NAME_PTR_INFO fields.
 
diff --git a/gcc/gimple-range.h b/gcc/gimple-range.h
index 5807a2b80e5..6b0835c4ca1 100644
--- a/gcc/gimple-range.h
+++ b/gcc/gimple-range.h
@@ -52,7 +52,6 @@ public:
   virtual bool range_of_stmt (vrange &r, gimple *, tree name = NULL) override;
   virtual bool range_of_expr (vrange &r, tree name, gimple * = NULL) override;
   virtual bool range_on_edge (vrange &r, edge e, tree name) override;
-  virtual void update_stmt (gimple *) override;
   void range_on_entry (vrange &r, basic_block bb, tree name);
   void range_on_exit (vrange &r, basic_block bb, tree name);
   void export_global_ranges ();
diff --git a/gcc/tree-ssa-operands.cc b/gcc/tree-ssa-operands.cc
index 57e393ae164..b0516a00d64 100644
--- a/gcc/tree-ssa-operands.cc
+++ b/gcc/tree-ssa-operands.cc
@@ -30,7 +30,6 @@ along with GCC; see the file COPYING3.  If not see
 #include "stmt.h"
 #include "print-tree.h"
 #include "dumpfile.h"
-#include "value-query.h"
 
 
 /* This file contains the code required to manage the operands cache of the
@@ -1146,8 +1145,6 @@ update_stmt_operands (struct function *fn, gimple *stmt)
   gcc_assert (gimple_modified_p (stmt));
   operands_scanner (fn, stmt).build_ssa_operands ();
   gimple_set_modified (stmt, false);
-  // Inform the active range query an update has happened.
-  get_range_query (fn)->update_stmt (stmt);
 
   timevar_pop (TV_TREE_OPS);
 }
diff --git a/gcc/value-query.h b/gcc/value-query.h
index 429446b32eb..0a6f18b03f6 100644
--- a/gcc/value-query.h
+++ b/gcc/value-query.h
@@ -71,9 +71,6 @@ public:
   virtual bool range_on_edge (vrange &r, edge, tree expr);
   virtual bool range_of_stmt (vrange &r, gimple *, tree name = NULL);
 
-  // When the IL in a stmt is changed, call this for better results.
-  virtual void update_stmt (gimple *) { }
-
   // Query if there is any relation between SSA1 and SSA2.
   relation_kind query_relation (gimple *s, tree ssa1, tree ssa2,
 				bool get_range = true);
-- 
2.35.3

Re: [PATCH] tree-optimization/112843 - update_stmt doing wrong things

[Andrew MacLeod]

On 12/5/23 03:27, Richard Biener wrote:
> The following removes range_query::update_stmt and its single
> invocation from update_stmt_operands.  That function is not
> supposed to look beyond the raw stmt contents of the passed
> stmt since there's no guarantee about the rest of the IL.
>
> I've successfully bootstrapped & tested the update_stmt_operands
> hunk, now testing removal of the actual routine as well.  The
> testcase that was added when introducing range_query::update_stmt
> still passes.
>
> OK to remove the implementation?  I don't see any way around
> removing the call though.

Im ok removing it.  Now that we are enabling ranger during a lot of IL 
updating, it probably doesn't make sense for the few cases it use to 
help with with, and may well be dangerous.

the testcase in question that was added appears to be threaded now which 
it wasn't before.  If a similar situation occurs and we need some sort 
of updating, I'll just mark the ssa-name on the LHS as out-of-date, and 
then it'll get lazily updated if need be.

Thanks.

Andrew

> Thanks,
> Richard.
>
> 	PR tree-optimization/112843
> 	* tree-ssa-operands.cc (update_stmt_operands): Do not call
> 	update_stmt from ranger.
> 	* value-query.h (range_query::update_stmt): Remove.
> 	* gimple-range.h (gimple_ranger::update_stmt): Likewise.
> 	* gimple-range.cc (gimple_ranger::update_stmt): Likewise.
> ---
>   gcc/gimple-range.cc      | 34 ----------------------------------
>   gcc/gimple-range.h       |  1 -
>   gcc/tree-ssa-operands.cc |  3 ---
>   gcc/value-query.h        |  3 ---
>   4 files changed, 41 deletions(-)
>
> diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
> index 5e9bb397a20..84d2c7516e6 100644
> --- a/gcc/gimple-range.cc
> +++ b/gcc/gimple-range.cc
> @@ -544,40 +544,6 @@ gimple_ranger::register_transitive_inferred_ranges (basic_block bb)
>       }
>   }
>   
> -// When a statement S has changed since the result was cached, re-evaluate
> -// and update the global cache.
> -
> -void
> -gimple_ranger::update_stmt (gimple *s)
> -{
> -  tree lhs = gimple_get_lhs (s);
> -  if (!lhs || !gimple_range_ssa_p (lhs))
> -    return;
> -  Value_Range r (TREE_TYPE (lhs));
> -  // Only update if it already had a value.
> -  if (m_cache.get_global_range (r, lhs))
> -    {
> -      // Re-calculate a new value using just cache values.
> -      Value_Range tmp (TREE_TYPE (lhs));
> -      fold_using_range f;
> -      fur_stmt src (s, &m_cache);
> -      f.fold_stmt (tmp, s, src, lhs);
> -
> -      // Combine the new value with the old value to check for a change.
> -      if (r.intersect (tmp))
> -	{
> -	  if (dump_file && (dump_flags & TDF_DETAILS))
> -	    {
> -	      print_generic_expr (dump_file, lhs, TDF_SLIM);
> -	      fprintf (dump_file, " : global value re-evaluated to ");
> -	      r.dump (dump_file);
> -	      fputc ('\n', dump_file);
> -	    }
> -	  m_cache.set_global_range (lhs, r);
> -	}
> -    }
> -}
> -
>   // This routine will export whatever global ranges are known to GCC
>   // SSA_RANGE_NAME_INFO and SSA_NAME_PTR_INFO fields.
>   
> diff --git a/gcc/gimple-range.h b/gcc/gimple-range.h
> index 5807a2b80e5..6b0835c4ca1 100644
> --- a/gcc/gimple-range.h
> +++ b/gcc/gimple-range.h
> @@ -52,7 +52,6 @@ public:
>     virtual bool range_of_stmt (vrange &r, gimple *, tree name = NULL) override;
>     virtual bool range_of_expr (vrange &r, tree name, gimple * = NULL) override;
>     virtual bool range_on_edge (vrange &r, edge e, tree name) override;
> -  virtual void update_stmt (gimple *) override;
>     void range_on_entry (vrange &r, basic_block bb, tree name);
>     void range_on_exit (vrange &r, basic_block bb, tree name);
>     void export_global_ranges ();
> diff --git a/gcc/tree-ssa-operands.cc b/gcc/tree-ssa-operands.cc
> index 57e393ae164..b0516a00d64 100644
> --- a/gcc/tree-ssa-operands.cc
> +++ b/gcc/tree-ssa-operands.cc
> @@ -30,7 +30,6 @@ along with GCC; see the file COPYING3.  If not see
>   #include "stmt.h"
>   #include "print-tree.h"
>   #include "dumpfile.h"
> -#include "value-query.h"
>   
>   
>   /* This file contains the code required to manage the operands cache of the
> @@ -1146,8 +1145,6 @@ update_stmt_operands (struct function *fn, gimple *stmt)
>     gcc_assert (gimple_modified_p (stmt));
>     operands_scanner (fn, stmt).build_ssa_operands ();
>     gimple_set_modified (stmt, false);
> -  // Inform the active range query an update has happened.
> -  get_range_query (fn)->update_stmt (stmt);
>   
>     timevar_pop (TV_TREE_OPS);
>   }
> diff --git a/gcc/value-query.h b/gcc/value-query.h
> index 429446b32eb..0a6f18b03f6 100644
> --- a/gcc/value-query.h
> +++ b/gcc/value-query.h
> @@ -71,9 +71,6 @@ public:
>     virtual bool range_on_edge (vrange &r, edge, tree expr);
>     virtual bool range_of_stmt (vrange &r, gimple *, tree name = NULL);
>   
> -  // When the IL in a stmt is changed, call this for better results.
> -  virtual void update_stmt (gimple *) { }
> -
>     // Query if there is any relation between SSA1 and SSA2.
>     relation_kind query_relation (gimple *s, tree ssa1, tree ssa2,
>   				bool get_range = true);