diff mbox series

[1/4] Add verification of SRA accesses

Message ID ri68snbp1kg.fsf@suse.cz
State New
Headers show
Series [1/4] Add verification of SRA accesses | expand

Commit Message

Martin Jambor Dec. 17, 2019, 12:40 p.m. UTC
Hi,

because the follow-up patches perform some non-trivial operations on
SRA patches, I wrote myself a verifier.  And sure enough, it has
spotted two issues, one of which is fixed in this patch too - we did
not correctly set the parent link when creating artificial accesses
for propagation across assignments.  The second one is the (not)
setting of reverse flag when creating accesses for total scalarization
but since the following patch removes the offending function, this
patch does not fix it.

Bootstrapped and tested on x86_64, I consider this a pre-requisite for
the followup patches (and the parent link fix really is).

Thanks,

Martin

2019-12-10  Martin Jambor  <mjambor@suse.cz>

	* tree-sra.c (verify_sra_access_forest): New function.
	(verify_all_sra_access_forests): Likewise.
	(create_artificial_child_access): Set parent.
	(analyze_all_variable_accesses): Call the verifier.
---
 gcc/tree-sra.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

Comments

Richard Biener Dec. 18, 2019, 11:11 a.m. UTC | #1
On December 17, 2019 1:40:47 PM GMT+01:00, Martin Jambor <mjambor@suse.cz> wrote:
>Hi,
>
>because the follow-up patches perform some non-trivial operations on
>SRA patches, I wrote myself a verifier.  And sure enough, it has
>spotted two issues, one of which is fixed in this patch too - we did
>not correctly set the parent link when creating artificial accesses
>for propagation across assignments.  The second one is the (not)
>setting of reverse flag when creating accesses for total scalarization
>but since the following patch removes the offending function, this
>patch does not fix it.
>
>Bootstrapped and tested on x86_64, I consider this a pre-requisite for
>the followup patches (and the parent link fix really is).

OK. 

Thanks 
Richard. 

>Thanks,
>
>Martin
>
>2019-12-10  Martin Jambor  <mjambor@suse.cz>
>
>	* tree-sra.c (verify_sra_access_forest): New function.
>	(verify_all_sra_access_forests): Likewise.
>	(create_artificial_child_access): Set parent.
>	(analyze_all_variable_accesses): Call the verifier.
>---
> gcc/tree-sra.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 86 insertions(+)
>
>diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
>index 87c156f2f54..e077a811da9 100644
>--- a/gcc/tree-sra.c
>+++ b/gcc/tree-sra.c
>@@ -2321,6 +2321,88 @@ build_access_trees (struct access *access)
>   return true;
> }
> 
>+/* Traverse the access forest where ROOT is the first root and verify
>that
>+   various important invariants hold true.  */
>+
>+DEBUG_FUNCTION void
>+verify_sra_access_forest (struct access *root)
>+{
>+  struct access *access = root;
>+  tree first_base = root->base;
>+  gcc_assert (DECL_P (first_base));
>+  do
>+    {
>+      gcc_assert (access->base == first_base);
>+      if (access->parent)
>+	gcc_assert (access->offset >= access->parent->offset
>+		    && access->size <= access->parent->size);
>+      if (access->next_sibling)
>+	gcc_assert (access->next_sibling->offset
>+		    >= access->offset + access->size);
>+
>+      poly_int64 poffset, psize, pmax_size;
>+      bool reverse;
>+      tree base = get_ref_base_and_extent (access->expr, &poffset,
>&psize,
>+					   &pmax_size, &reverse);
>+      HOST_WIDE_INT offset, size, max_size;
>+      if (!poffset.is_constant (&offset)
>+	  || !psize.is_constant (&size)
>+	  || !pmax_size.is_constant (&max_size))
>+	gcc_unreachable ();
>+      gcc_assert (base == first_base);
>+      gcc_assert (offset == access->offset);
>+      gcc_assert (access->grp_unscalarizable_region
>+		  || size == max_size);
>+      gcc_assert (max_size == access->size);
>+      gcc_assert (reverse == access->reverse);
>+
>+      if (access->first_child)
>+	{
>+	  gcc_assert (access->first_child->parent == access);
>+	  access = access->first_child;
>+	}
>+      else if (access->next_sibling)
>+	{
>+	  gcc_assert (access->next_sibling->parent == access->parent);
>+	  access = access->next_sibling;
>+	}
>+      else
>+	{
>+	  while (access->parent && !access->next_sibling)
>+	    access = access->parent;
>+	  if (access->next_sibling)
>+	    access = access->next_sibling;
>+	  else
>+	    {
>+	      gcc_assert (access == root);
>+	      root = root->next_grp;
>+	      access = root;
>+	    }
>+	}
>+    }
>+  while (access);
>+}
>+
>+/* Verify access forests of all candidates with accesses by calling
>+   verify_access_forest on each on them.  */
>+
>+DEBUG_FUNCTION void
>+verify_all_sra_access_forests (void)
>+{
>+  bitmap_iterator bi;
>+  unsigned i;
>+  EXECUTE_IF_SET_IN_BITMAP (candidate_bitmap, 0, i, bi)
>+    {
>+      tree var = candidate (i);
>+      struct access *access = get_first_repr_for_decl (var);
>+      if (access)
>+	{
>+	  gcc_assert (access->base == var);
>+	  verify_sra_access_forest (access);
>+	}
>+    }
>+}
>+
>/* Return true if expr contains some ARRAY_REFs into a variable bounded
>    array.  */
> 
>@@ -2566,6 +2648,7 @@ create_artificial_child_access (struct access
>*parent, struct access *model,
>   access->offset = new_offset;
>   access->size = model->size;
>   access->type = model->type;
>+  access->parent = parent;
>   access->grp_write = set_grp_write;
>   access->grp_read = false;
>   access->reverse = model->reverse;
>@@ -2850,6 +2933,9 @@ analyze_all_variable_accesses (void)
> 
>   propagate_all_subaccesses ();
> 
>+  if (flag_checking)
>+    verify_all_sra_access_forests ();
>+
>   bitmap_copy (tmp, candidate_bitmap);
>   EXECUTE_IF_SET_IN_BITMAP (tmp, 0, i, bi)
>     {
diff mbox series

Patch

diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 87c156f2f54..e077a811da9 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -2321,6 +2321,88 @@  build_access_trees (struct access *access)
   return true;
 }
 
+/* Traverse the access forest where ROOT is the first root and verify that
+   various important invariants hold true.  */
+
+DEBUG_FUNCTION void
+verify_sra_access_forest (struct access *root)
+{
+  struct access *access = root;
+  tree first_base = root->base;
+  gcc_assert (DECL_P (first_base));
+  do
+    {
+      gcc_assert (access->base == first_base);
+      if (access->parent)
+	gcc_assert (access->offset >= access->parent->offset
+		    && access->size <= access->parent->size);
+      if (access->next_sibling)
+	gcc_assert (access->next_sibling->offset
+		    >= access->offset + access->size);
+
+      poly_int64 poffset, psize, pmax_size;
+      bool reverse;
+      tree base = get_ref_base_and_extent (access->expr, &poffset, &psize,
+					   &pmax_size, &reverse);
+      HOST_WIDE_INT offset, size, max_size;
+      if (!poffset.is_constant (&offset)
+	  || !psize.is_constant (&size)
+	  || !pmax_size.is_constant (&max_size))
+	gcc_unreachable ();
+      gcc_assert (base == first_base);
+      gcc_assert (offset == access->offset);
+      gcc_assert (access->grp_unscalarizable_region
+		  || size == max_size);
+      gcc_assert (max_size == access->size);
+      gcc_assert (reverse == access->reverse);
+
+      if (access->first_child)
+	{
+	  gcc_assert (access->first_child->parent == access);
+	  access = access->first_child;
+	}
+      else if (access->next_sibling)
+	{
+	  gcc_assert (access->next_sibling->parent == access->parent);
+	  access = access->next_sibling;
+	}
+      else
+	{
+	  while (access->parent && !access->next_sibling)
+	    access = access->parent;
+	  if (access->next_sibling)
+	    access = access->next_sibling;
+	  else
+	    {
+	      gcc_assert (access == root);
+	      root = root->next_grp;
+	      access = root;
+	    }
+	}
+    }
+  while (access);
+}
+
+/* Verify access forests of all candidates with accesses by calling
+   verify_access_forest on each on them.  */
+
+DEBUG_FUNCTION void
+verify_all_sra_access_forests (void)
+{
+  bitmap_iterator bi;
+  unsigned i;
+  EXECUTE_IF_SET_IN_BITMAP (candidate_bitmap, 0, i, bi)
+    {
+      tree var = candidate (i);
+      struct access *access = get_first_repr_for_decl (var);
+      if (access)
+	{
+	  gcc_assert (access->base == var);
+	  verify_sra_access_forest (access);
+	}
+    }
+}
+
 /* Return true if expr contains some ARRAY_REFs into a variable bounded
    array.  */
 
@@ -2566,6 +2648,7 @@  create_artificial_child_access (struct access *parent, struct access *model,
   access->offset = new_offset;
   access->size = model->size;
   access->type = model->type;
+  access->parent = parent;
   access->grp_write = set_grp_write;
   access->grp_read = false;
   access->reverse = model->reverse;
@@ -2850,6 +2933,9 @@  analyze_all_variable_accesses (void)
 
   propagate_all_subaccesses ();
 
+  if (flag_checking)
+    verify_all_sra_access_forests ();
+
   bitmap_copy (tmp, candidate_bitmap);
   EXECUTE_IF_SET_IN_BITMAP (tmp, 0, i, bi)
     {