[COMMITTED 1/2] Adjust on_entry cache to indicate if the value was set properly.

[COMMITTED 1/2] Adjust on_entry cache to indicate if the value was set properly.

[Andrew MacLeod]

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

The introduction of the sparse bitmap on-entry cache for large BB 
functions also introduced the concept that the value we ask to be 
written to the cache may not be properly represented.  It is limited to 
15 unique ranges for any given ssa-name, then  it reverts to varying for 
any additional values to be safe.

This patch adds a boolean return value to the cache set routines, 
allowing us to check if the value was properly written.

Other than that, no functional change.

Bootstrap on x86_64-pc-linux-gnu with no regressions.  pushed.

Andrew


[-- Attachment #2: 0002-Adjust-on_entry-cache-to-indicate-if-the-value-was-s.patch --]
[-- Type: text/x-patch, Size: 5937 bytes --]

From ca4d381662c37733b2a1d49d6c8f5fcfc1348f3d Mon Sep 17 00:00:00 2001
From: Andrew MacLeod <amacleod@redhat.com>
Date: Tue, 22 Jun 2021 17:21:32 -0400
Subject: [PATCH 2/4] Adjust on_entry cache to indicate if the value was set
 properly.

	* gimple-range-cache.cc (class ssa_block_ranges): Adjust prototype.
	(sbr_vector::set_bb_range): Return true.
	(class sbr_sparse_bitmap): Adjust.
	(sbr_sparse_bitmap::set_bb_range): Return value.
	(block_range_cache::set_bb_range): Return value.
	(ranger_cache::propagate_cache): Use return value to print msg.
	* gimple-range-cache.h (class block_range_cache): Adjust.
---
 gcc/gimple-range-cache.cc | 44 ++++++++++++++++++++++-----------------
 gcc/gimple-range-cache.h  |  2 +-
 2 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/gcc/gimple-range-cache.cc b/gcc/gimple-range-cache.cc
index 4347485cf98..bdecd212691 100644
--- a/gcc/gimple-range-cache.cc
+++ b/gcc/gimple-range-cache.cc
@@ -132,7 +132,7 @@ non_null_ref::process_name (tree name)
 class ssa_block_ranges
 {
 public:
-  virtual void set_bb_range (const basic_block bb, const irange &r) = 0;
+  virtual bool set_bb_range (const basic_block bb, const irange &r) = 0;
   virtual bool get_bb_range (irange &r, const basic_block bb) = 0;
   virtual bool bb_range_p (const basic_block bb) = 0;
 
@@ -165,7 +165,7 @@ class sbr_vector : public ssa_block_ranges
 public:
   sbr_vector (tree t, irange_allocator *allocator);
 
-  virtual void set_bb_range (const basic_block bb, const irange &r) OVERRIDE;
+  virtual bool set_bb_range (const basic_block bb, const irange &r) OVERRIDE;
   virtual bool get_bb_range (irange &r, const basic_block bb) OVERRIDE;
   virtual bool bb_range_p (const basic_block bb) OVERRIDE;
 protected:
@@ -196,7 +196,7 @@ sbr_vector::sbr_vector (tree t, irange_allocator *allocator)
 
 // Set the range for block BB to be R.
 
-void
+bool
 sbr_vector::set_bb_range (const basic_block bb, const irange &r)
 {
   irange *m;
@@ -208,6 +208,7 @@ sbr_vector::set_bb_range (const basic_block bb, const irange &r)
   else
     m = m_irange_allocator->allocate (r);
   m_tab[bb->index] = m;
+  return true;
 }
 
 // Return the range associated with block BB in R.  Return false if
@@ -252,7 +253,7 @@ class sbr_sparse_bitmap : public ssa_block_ranges
 {
 public:
   sbr_sparse_bitmap (tree t, irange_allocator *allocator, bitmap_obstack *bm);
-  virtual void set_bb_range (const basic_block bb, const irange &r) OVERRIDE;
+  virtual bool set_bb_range (const basic_block bb, const irange &r) OVERRIDE;
   virtual bool get_bb_range (irange &r, const basic_block bb) OVERRIDE;
   virtual bool bb_range_p (const basic_block bb) OVERRIDE;
 private:
@@ -312,13 +313,13 @@ sbr_sparse_bitmap::bitmap_get_quad (const_bitmap head, int quad)
 
 // Set the range on entry to basic block BB to R.
 
-void
+bool
 sbr_sparse_bitmap::set_bb_range (const basic_block bb, const irange &r)
 {
   if (r.undefined_p ())
     {
       bitmap_set_quad (bitvec, bb->index, SBR_UNDEF);
-      return;
+      return true;
     }
 
   // Loop thru the values to see if R is already present.
@@ -328,11 +329,11 @@ sbr_sparse_bitmap::set_bb_range (const basic_block bb, const irange &r)
 	if (!m_range[x])
 	  m_range[x] = m_irange_allocator->allocate (r);
 	bitmap_set_quad (bitvec, bb->index, x + 1);
-	return;
+	return true;
       }
   // All values are taken, default to VARYING.
   bitmap_set_quad (bitvec, bb->index, SBR_VARYING);
-  return;
+  return false;
 }
 
 // Return the range associated with block BB in R.  Return false if
@@ -387,7 +388,7 @@ block_range_cache::~block_range_cache ()
 // Set the range for NAME on entry to block BB to R.
 // If it has not been accessed yet, allocate it first.
 
-void
+bool
 block_range_cache::set_bb_range (tree name, const basic_block bb,
 				 const irange &r)
 {
@@ -413,7 +414,7 @@ block_range_cache::set_bb_range (tree name, const basic_block bb,
 						m_irange_allocator);
 	}
     }
-  m_ssa_ranges[v]->set_bb_range (bb, r);
+  return m_ssa_ranges[v]->set_bb_range (bb, r);
 }
 
 
@@ -1061,13 +1062,18 @@ ranger_cache::propagate_cache (tree name)
       // If the range on entry has changed, update it.
       if (new_range != current_range)
 	{
+	  bool ok_p = m_on_entry.set_bb_range (name, bb, new_range);
 	  if (DEBUG_RANGE_CACHE) 
 	    {
-	      fprintf (dump_file, "      Updating range to ");
-	      new_range.dump (dump_file);
+	      if (!ok_p)
+		fprintf (dump_file, "     Cache failure to store value.");
+	      else
+		{
+		  fprintf (dump_file, "      Updating range to ");
+		  new_range.dump (dump_file);
+		}
 	      fprintf (dump_file, "\n      Updating blocks :");
 	    }
-	  m_on_entry.set_bb_range (name, bb, new_range);
 	  // Mark each successor that has a range to re-check its range
 	  FOR_EACH_EDGE (e, ei, bb->succs)
 	    if (m_on_entry.bb_range_p (name, e->dest))
@@ -1080,12 +1086,12 @@ ranger_cache::propagate_cache (tree name)
 	    fprintf (dump_file, "\n");
 	}
     }
-    if (DEBUG_RANGE_CACHE)
-      {
-	fprintf (dump_file, "DONE visiting blocks for ");
-	print_generic_expr (dump_file, name, TDF_SLIM);
-	fprintf (dump_file, "\n");
-      }
+  if (DEBUG_RANGE_CACHE)
+    {
+      fprintf (dump_file, "DONE visiting blocks for ");
+      print_generic_expr (dump_file, name, TDF_SLIM);
+      fprintf (dump_file, "\n");
+    }
 }
 
 // Check to see if an update to the value for NAME in BB has any effect
diff --git a/gcc/gimple-range-cache.h b/gcc/gimple-range-cache.h
index 04150ea54a8..1d2e1b99200 100644
--- a/gcc/gimple-range-cache.h
+++ b/gcc/gimple-range-cache.h
@@ -50,7 +50,7 @@ public:
   block_range_cache ();
   ~block_range_cache ();
 
-  void set_bb_range (tree name, const basic_block bb, const irange &r);
+  bool set_bb_range (tree name, const basic_block bb, const irange &r);
   bool get_bb_range (irange &r, tree name, const basic_block bb);
   bool bb_range_p (tree name, const basic_block bb);
 
-- 
2.17.2

Re: [COMMITTED 1/2] Adjust on_entry cache to indicate if the value was set properly.

[Martin Sebor]

I just glanced at the patch out of curiosity and the first hunk
caught my eye.  There's nothing wrong with the change and I like
how you make the APIs const-correct!  Just a note that it looks
like the const in on the basic_block declaration might be missing
an underscore (it should be const_basic_block).  Otherwise it's
superfluous.  This is repeated in a bunch of places in the file
and its header.

Martin

diff --git a/gcc/gimple-range-cache.cc b/gcc/gimple-range-cache.cc
index 4347485cf98..bdecd212691 100644
--- a/gcc/gimple-range-cache.cc
+++ b/gcc/gimple-range-cache.cc
@@ -132,7 +132,7 @@ non_null_ref::process_name (tree name)
  class ssa_block_ranges
  {
  public:
-  virtual void set_bb_range (const basic_block bb, const irange &r) = 0;
+  virtual bool set_bb_range (const basic_block bb, const irange &r) = 0;
    virtual bool get_bb_range (irange &r, const basic_block bb) = 0;
    virtual bool bb_range_p (const basic_block bb) = 0;

On 6/23/21 8:27 AM, Andrew MacLeod via Gcc-patches wrote:
> The introduction of the sparse bitmap on-entry cache for large BB 
> functions also introduced the concept that the value we ask to be 
> written to the cache may not be properly represented.  It is limited to 
> 15 unique ranges for any given ssa-name, then  it reverts to varying for 
> any additional values to be safe.
> 
> This patch adds a boolean return value to the cache set routines, 
> allowing us to check if the value was properly written.
> 
> Other than that, no functional change.
> 
> Bootstrap on x86_64-pc-linux-gnu with no regressions.  pushed.
> 
> Andrew
>