| Message ID | ri6pnc8ydbl.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | sra: Fix access verification (PR 94598) | expand |
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) >
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.
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)