[COMMITTED 1/5] PR tree-optimization/109417 - Don't save ssa-name pointer in dependency cache.

[Andrew MacLeod <amacleod@redhat.com>]

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


On 4/25/23 22:34, Jeff Law wrote:
>
>
> On 4/24/23 07:51, Andrew MacLeod wrote:
>
>>
>> Its not a real cache..  its merely a statement shortcut in dependency 
>> analysis to avoid re-parsing statements every time we look at them 
>> for dependency analysis
>>
>> It is not suppose to be used for anything other than dependency 
>> checking.   ie, if an SSA_NAME changes, we can check if it matches 
>> either of the 2 "cached" names on this DEF, and if so, we know this 
>> name is stale.  we are never actually suppose to use the dependency 
>> cached values to drive anything, merely respond to the question if 
>> either matches a given name.   So it doesnt matter if the name here 
>> has been freed
> OK.  I'll take your word for it.  Note that a free'd SSA_NAME may have 
> an empty TREE_TYPE or an unexpected TREE_CHAIN field IIRC. So you have 
> to be a bit careful if you're going to allow them.
>
>>
>>
>>> We never re-use SSA names from within the pass releasing it.  But if
>>> the ranger cache
>>> persists across passes this could be a problem.  See
>>
>>
>> This particular valueswould never persist beyond a current pass.. its 
>> just the dependency chains and they would get rebuilt every time 
>> because the IL has changed.
> Good.  THat would limit the concerns significantly.  I don't think we 
> recycle names within a pass anymore (we used to within DOM due to the 
> way threading worked eons ago, but we no longer take things out of SSA 
> form to handle the CFG/SSA graph updates.  One could even argue we 
> don't need to maintain the freelist and recycle names anymore.
>
> Jeff
>
well, no worries.  taken care of thusly for the future. Its a hair 
slower, but nothing outrageous

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

Andrew





[-- Attachment #2: 0001-Don-t-save-ssa-name-pointer-in-dependency-cache.patch --]
[-- Type: text/x-patch, Size: 3805 bytes --]

From a530eb642032da7ad4d30de51131421631055f72 Mon Sep 17 00:00:00 2001
From: Andrew MacLeod <amacleod@redhat.com>
Date: Tue, 25 Apr 2023 15:33:52 -0400
Subject: [PATCH 1/5] Don't save ssa-name pointer in dependency cache.

If the direct dependence fields point directly to an ssa-name,
its possible that an optimization frees an ssa-name, and the value
pointed to may now be in the free list.   Simply maintain the ssa
version number instead.

	PR tree-optimization/109417
	* gimple-range-gori.cc (range_def_chain::register_dependency):
	Save the ssa version number, not the pointer.
	(gori_compute::may_recompute_p): No need to check if a dependency
	is in the free list.
	* gimple-range-gori.h (class range_def_chain): Change ssa1 and ssa2
	fields to be unsigned int instead of trees.
	(ange_def_chain::depend1): Adjust.
	(ange_def_chain::depend2): Adjust.
	* gimple-range.h: Include "ssa.h" to inline ssa_name().
---
 gcc/gimple-range-gori.cc |  8 ++++----
 gcc/gimple-range-gori.h  | 14 ++++++++++----
 gcc/gimple-range.h       |  1 +
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/gcc/gimple-range-gori.cc b/gcc/gimple-range-gori.cc
index d77e1f51ac2..5bba77c7b7b 100644
--- a/gcc/gimple-range-gori.cc
+++ b/gcc/gimple-range-gori.cc
@@ -182,9 +182,9 @@ range_def_chain::register_dependency (tree name, tree dep, basic_block bb)
 
   // Set the direct dependency cache entries.
   if (!src.ssa1)
-    src.ssa1 = dep;
-  else if (!src.ssa2 && src.ssa1 != dep)
-    src.ssa2 = dep;
+    src.ssa1 = SSA_NAME_VERSION (dep);
+  else if (!src.ssa2 && src.ssa1 != SSA_NAME_VERSION (dep))
+    src.ssa2 = SSA_NAME_VERSION (dep);
 
   // Don't calculate imports or export/dep chains if BB is not provided.
   // This is usually the case for when the temporal cache wants the direct
@@ -1316,7 +1316,7 @@ gori_compute::may_recompute_p (tree name, basic_block bb, int depth)
   // If the first dependency is not set, there is no recomputation.
   // Dependencies reflect original IL, not current state.   Check if the
   // SSA_NAME is still valid as well.
-  if (!dep1 || SSA_NAME_IN_FREE_LIST (dep1))
+  if (!dep1)
     return false;
 
   // Don't recalculate PHIs or statements with side_effects.
diff --git a/gcc/gimple-range-gori.h b/gcc/gimple-range-gori.h
index 3ea4b45595b..526edc24b53 100644
--- a/gcc/gimple-range-gori.h
+++ b/gcc/gimple-range-gori.h
@@ -46,8 +46,8 @@ protected:
   bitmap_obstack m_bitmaps;
 private:
   struct rdc {
-   tree ssa1;		// First direct dependency
-   tree ssa2;		// Second direct dependency
+   unsigned int ssa1;		// First direct dependency
+   unsigned int ssa2;		// Second direct dependency
    bitmap bm;		// All dependencies
    bitmap m_import;
   };
@@ -66,7 +66,10 @@ range_def_chain::depend1 (tree name) const
   unsigned v = SSA_NAME_VERSION (name);
   if (v >= m_def_chain.length ())
     return NULL_TREE;
-  return m_def_chain[v].ssa1;
+  unsigned v1 = m_def_chain[v].ssa1;
+  if (!v1)
+    return NULL_TREE;
+  return ssa_name (v1);
 }
 
 // Return the second direct dependency for NAME, if there is one.
@@ -77,7 +80,10 @@ range_def_chain::depend2 (tree name) const
   unsigned v = SSA_NAME_VERSION (name);
   if (v >= m_def_chain.length ())
     return NULL_TREE;
-  return m_def_chain[v].ssa2;
+  unsigned v2 = m_def_chain[v].ssa2;
+  if (!v2)
+    return NULL_TREE;
+  return ssa_name (v2);
 }
 
 // GORI_MAP is used to accumulate what SSA names in a block can
diff --git a/gcc/gimple-range.h b/gcc/gimple-range.h
index 7ed4d3870b8..b8ddca59d2d 100644
--- a/gcc/gimple-range.h
+++ b/gcc/gimple-range.h
@@ -22,6 +22,7 @@ along with GCC; see the file COPYING3.  If not see
 #ifndef GCC_GIMPLE_RANGE_H
 #define GCC_GIMPLE_RANGE_H
 
+#include "ssa.h"
 #include "range.h"
 #include "value-query.h"
 #include "gimple-range-op.h"
-- 
2.39.2