[COMMITTED 3/3] PR tree-optimization/109695 - Only update global value if it changes.

[COMMITTED 3/3] PR tree-optimization/109695 - Only update global value if it changes.

[Andrew MacLeod]

[-- Attachment #1.1: Type: text/plain, Size: 2332 bytes --]

This patch implements suggestion 1) from the PR:

    1) We unconditionally write the new value calculated to the global
    cache once the dependencies are resolved.  This gives it a new
    timestamp, and thus makes any other values which used it out of date
    when they really aren't.   This causes a lot of extra churn.

    TODO: This should be changed to only update it when it actually
    changes.  Client code shouldn't have to do this, it should be
    handled right int the cache's set_global_value ().

It turns out it is about a 3% compilation speed hit to compare the 
ranges every time we set them, which loses any gains we see. As such, I 
changed it so that set_global_range takes an extra parameter which 
indicates whether the value has changed or not. In all cases, we have 
the result of intersection which gives us the information for free, so 
we might as well take advantage of it.  instead we get about a 2.7% 
improvement in speed in VRP and another 0.7% in threading.

set_global_range now checks the changed flag, and if it hasnt changed, 
checks to see if the value is current or not, and only gives the result 
a new timestamp if its out of date.  I found many cases where we
   1) initally calculate the result, give it a timestamp,
   2) then evaluate the dependencies.. which get fresher timestamps than 
the result
   3) the initial result turns out to still be right, so we dont have to 
propagate the value or change it.

However, if we do not give it a fresh time stamp in this case, it wil be 
out of date if we ever check is sine the dependencies are fresher.   So 
in this case, we give it a new timestamp so we wont re-evaluate it.

The 3 patches together result in VRP being just 0.15% slower, threading 
being 0.6% faster, and overall compilation improves by 0.05%

It will also compile the testcase from the PR with issues after 
reverting Aldy's memory work and using int_range_max as int_range<255> 
again.. so that is also an indication the results are worthwhile.

At this point, I don't think its worth pursuing suggestion 4 from the 
PR.. it is wrought with dependency issues that I don't think we need to 
deal with at this moment.  When I have more time I will give it more 
consideration.

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

Andrew

[-- Attachment #2: 0003-Only-update-global-value-if-it-changes.patch --]
[-- Type: text/x-patch, Size: 3291 bytes --]

From c3c1499498ff8f465ec7eacce6681c5c2da03a92 Mon Sep 17 00:00:00 2001
From: Andrew MacLeod <amacleod@redhat.com>
Date: Tue, 23 May 2023 15:41:03 -0400
Subject: [PATCH 3/3] Only update global value if it changes.

Do not update and propagate a global value if it hasn't changed.

	PR tree-optimization/109695
	* gimple-range-cache.cc (ranger_cache::get_global_range): Add
	changed param.
	* gimple-range-cache.h (ranger_cache::get_global_range): Ditto.
	* gimple-range.cc (gimple_ranger::range_of_stmt): Pass changed
	flag to set_global_range.
	(gimple_ranger::prefill_stmt_dependencies): Ditto.
---
 gcc/gimple-range-cache.cc | 10 +++++++++-
 gcc/gimple-range-cache.h  |  2 +-
 gcc/gimple-range.cc       |  8 ++++----
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/gcc/gimple-range-cache.cc b/gcc/gimple-range-cache.cc
index db7ee8eab4e..e069241bc9d 100644
--- a/gcc/gimple-range-cache.cc
+++ b/gcc/gimple-range-cache.cc
@@ -992,10 +992,18 @@ ranger_cache::get_global_range (vrange &r, tree name, bool &current_p)
 //  Set the global range of NAME to R and give it a timestamp.
 
 void
-ranger_cache::set_global_range (tree name, const vrange &r)
+ranger_cache::set_global_range (tree name, const vrange &r, bool changed)
 {
   // Setting a range always clears the always_current flag.
   m_temporal->set_always_current (name, false);
+  if (!changed)
+    {
+      // If there are dependencies, make sure this is not out of date.
+      if (!m_temporal->current_p (name, m_gori.depend1 (name),
+				 m_gori.depend2 (name)))
+	m_temporal->set_timestamp (name);
+      return;
+    }
   if (m_globals.set_range (name, r))
     {
       // If there was already a range set, propagate the new value.
diff --git a/gcc/gimple-range-cache.h b/gcc/gimple-range-cache.h
index 946fbc51465..871255a8116 100644
--- a/gcc/gimple-range-cache.h
+++ b/gcc/gimple-range-cache.h
@@ -117,7 +117,7 @@ public:
 
   bool get_global_range (vrange &r, tree name) const;
   bool get_global_range (vrange &r, tree name, bool &current_p);
-  void set_global_range (tree name, const vrange &r);
+  void set_global_range (tree name, const vrange &r, bool changed = true);
 
   void propagate_updated_value (tree name, basic_block bb);
 
diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index a275c090e4b..4fae3f95e6a 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -320,8 +320,8 @@ gimple_ranger::range_of_stmt (vrange &r, gimple *s, tree name)
       // Combine the new value with the old value.  This is required because
       // the way value propagation works, when the IL changes on the fly we
       // can sometimes get different results.  See PR 97741.
-      r.intersect (tmp);
-      m_cache.set_global_range (name, r);
+      bool changed = r.intersect (tmp);
+      m_cache.set_global_range (name, r, changed);
       res = true;
     }
 
@@ -393,8 +393,8 @@ gimple_ranger::prefill_stmt_dependencies (tree ssa)
 	      // Make sure we don't lose any current global info.
 	      Value_Range tmp (TREE_TYPE (name));
 	      m_cache.get_global_range (tmp, name);
-	      r.intersect (tmp);
-	      m_cache.set_global_range (name, r);
+	      bool changed = tmp.intersect (r);
+	      m_cache.set_global_range (name, tmp, changed);
 	    }
 	  continue;
 	}
-- 
2.40.1