sra: Fix access verification (PR 94598)

Message ID ri6pnc8ydbl.fsf@suse.cz
State New
Headers show
Series
  • sra: Fix access verification (PR 94598)
Related show

Commit Message

Martin Jambor April 15, 2020, 3:32 p.m.
Hi,

get_ref_base_and_extent recognizes ARRAY_REFs with variable index but
into arrays of length one as constant offset accesses.  However,
max_size in such cases is extended to span the whole element.  This
confuses SRA verification when SRA also builds its (total
scalarization) access structures to describe fields under such array -
get_ref_base_and_extent returns different size and max_size for them.

Fixed by not performing the check for total scalarization accesses.
The subsequent check then had to be changed to use size and not
max_size too, which meant it has to be skipped when the access
structure describes a genuine variable array access.

Bootstrapped and tested on x86_64-linux.

OK for trunk?

Thanks,

Martin


2020-04-15  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/94598
	* tree-sra.c (verify_sra_access_forest): Fix verification of total
	scalarization accesses under access to one-element arrays.

	testsuite/
	* gcc.dg/tree-ssa/pr94598.c: New test.
---
 gcc/ChangeLog                           |  6 ++++++
 gcc/testsuite/ChangeLog                 |  5 +++++
 gcc/testsuite/gcc.dg/tree-ssa/pr94598.c | 26 +++++++++++++++++++++++++
 gcc/tree-sra.c                          |  6 ++++--
 4 files changed, 41 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr94598.c

-- 
2.26.0

Comments

Richard Biener April 15, 2020, 6:15 p.m. | #1
On Wed, 15 Apr 2020, Martin Jambor wrote:

> Hi,

> 

> get_ref_base_and_extent recognizes ARRAY_REFs with variable index but

> into arrays of length one as constant offset accesses.  However,

> max_size in such cases is extended to span the whole element.


You mean f[d] gets offset zero and max_size == sizeof (struct a)?
Or f[d].b doing this, thus size != max_size?

> This confuses SRA verification when SRA also builds its (total

> scalarization) access structures to describe fields under such array -

> get_ref_base_and_extent returns different size and max_size for them.

> 

> Fixed by not performing the check for total scalarization accesses.

> The subsequent check then had to be changed to use size and not

> max_size too, which meant it has to be skipped when the access

> structure describes a genuine variable array access.

> 

> Bootstrapped and tested on x86_64-linux.

> 

> OK for trunk?


OK.

Richard.

> Thanks,

> 

> Martin

> 

> 

> 2020-04-15  Martin Jambor  <mjambor@suse.cz>

> 

> 	PR tree-optimization/94598

> 	* tree-sra.c (verify_sra_access_forest): Fix verification of total

> 	scalarization accesses under access to one-element arrays.

> 

> 	testsuite/

> 	* gcc.dg/tree-ssa/pr94598.c: New test.

> ---

>  gcc/ChangeLog                           |  6 ++++++

>  gcc/testsuite/ChangeLog                 |  5 +++++

>  gcc/testsuite/gcc.dg/tree-ssa/pr94598.c | 26 +++++++++++++++++++++++++

>  gcc/tree-sra.c                          |  6 ++++--

>  4 files changed, 41 insertions(+), 2 deletions(-)

>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr94598.c

> 

> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr94598.c b/gcc/testsuite/gcc.dg/tree-ssa/pr94598.c

> new file mode 100644

> index 00000000000..a18a796aa0a

> --- /dev/null

> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr94598.c

> @@ -0,0 +1,26 @@

> +/* { dg-do compile } */

> +/* { dg-options "-O1" } */

> +

> +struct a {

> +  int b;

> +  short c;

> +};

> +int d;

> +void e() {

> +  struct a f[1];

> +  f[d] = f[d];

> +}

> +

> +struct S {

> +  int a[30];

> +  int c;

> +};

> +

> +int get_int (void);

> +

> +int foo(struct S p)

> +{

> +  p.c = get_int ();

> +  p.a[get_int()] = get_int()+1;

> +  return p.c;

> +}

> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c

> index 84c113c403c..227bde06257 100644

> --- a/gcc/tree-sra.c

> +++ b/gcc/tree-sra.c

> @@ -2357,9 +2357,11 @@ verify_sra_access_forest (struct access *root)

>        gcc_assert (base == first_base);

>        gcc_assert (offset == access->offset);

>        gcc_assert (access->grp_unscalarizable_region

> +		  || access->grp_total_scalarization

>  		  || size == max_size);

> -      gcc_assert (!is_gimple_reg_type (access->type)

> -		  || max_size == access->size);

> +      gcc_assert (access->grp_unscalarizable_region

> +		  || !is_gimple_reg_type (access->type)

> +		  || size == access->size);

>        gcc_assert (reverse == access->reverse);

>  

>        if (access->first_child)

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
Martin Jambor April 16, 2020, 9:38 a.m. | #2
Hi,

On Wed, Apr 15 2020, Richard Biener wrote:
> On Wed, 15 Apr 2020, Martin Jambor wrote:

>

>> Hi,

>> 

>> get_ref_base_and_extent recognizes ARRAY_REFs with variable index but

>> into arrays of length one as constant offset accesses.  However,

>> max_size in such cases is extended to span the whole element.

>

> You mean f[d] gets offset zero and max_size == sizeof (struct a)?

> Or f[d].b doing this, thus size != max_size?


in GCC 10 we scan function body first and only then try to fill in the
gaps if/when doing total scalarization.  So during body scan we
encounter f[d] which gets offset 0, size equal to the element size
64, and max_size equal to array size minus offset.  Because array length
is 1, size of the array is 64 and thus max_size is 64.  SRA sees size ==
max_size and is satisfied (size != max_size generally indicates an array
access that is unusable for SRA).

Then total scalarization comes along.  First, it determines whether it
needs to create an access for f[0] but there already is an access with
offset 0 and size 64 so it reuses that.  But then under this one it
creates sub-accesses for f[d].b and f[d].c - and re-uses the expression 
of their parent for their expression and so the variable index remains.

Then verification happened, ran get_ref_base_and_extent on f[d].b, which
has offset zero, size 32 but max_size is the size of the array minus
offset, so 64.  And SRA verification though this should have never been
a scalarizable access in the first place and complained that
grp_unscalarizable_region was not set.

During total scalarization we could take extra care to create expression
f[0].b and f[0].c instead but at this stage I decided to "fix" it the
verifier.  ATM it does not affect anything else.  In future we might
actually want to replace the index already during body scan to make
these candidates for same_access_path_p() test so that whenever these
expressions are re-used, the results are friendlier to the alias oracle
- see the TODO in path_comparable_for_same_access.

>

>> This confuses SRA verification when SRA also builds its (total

>> scalarization) access structures to describe fields under such array -

>> get_ref_base_and_extent returns different size and max_size for them.

>> 

>> Fixed by not performing the check for total scalarization accesses.

>> The subsequent check then had to be changed to use size and not

>> max_size too, which meant it has to be skipped when the access

>> structure describes a genuine variable array access.

>> 

>> Bootstrapped and tested on x86_64-linux.

>> 

>> OK for trunk?

>

> OK.

>


Thanks, I have just pushed the patch to master.

Martin


>> 

>> 

>> 2020-04-15  Martin Jambor  <mjambor@suse.cz>

>> 

>> 	PR tree-optimization/94598

>> 	* tree-sra.c (verify_sra_access_forest): Fix verification of total

>> 	scalarization accesses under access to one-element arrays.

>> 

>> 	testsuite/

>> 	* gcc.dg/tree-ssa/pr94598.c: New test.

Patch

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr94598.c b/gcc/testsuite/gcc.dg/tree-ssa/pr94598.c
new file mode 100644
index 00000000000..a18a796aa0a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr94598.c
@@ -0,0 +1,26 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O1" } */
+
+struct a {
+  int b;
+  short c;
+};
+int d;
+void e() {
+  struct a f[1];
+  f[d] = f[d];
+}
+
+struct S {
+  int a[30];
+  int c;
+};
+
+int get_int (void);
+
+int foo(struct S p)
+{
+  p.c = get_int ();
+  p.a[get_int()] = get_int()+1;
+  return p.c;
+}
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 84c113c403c..227bde06257 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -2357,9 +2357,11 @@  verify_sra_access_forest (struct access *root)
       gcc_assert (base == first_base);
       gcc_assert (offset == access->offset);
       gcc_assert (access->grp_unscalarizable_region
+		  || access->grp_total_scalarization
 		  || size == max_size);
-      gcc_assert (!is_gimple_reg_type (access->type)
-		  || max_size == access->size);
+      gcc_assert (access->grp_unscalarizable_region
+		  || !is_gimple_reg_type (access->type)
+		  || size == access->size);
       gcc_assert (reverse == access->reverse);
 
       if (access->first_child)