Re: [PATCH 2/5] completely_scalarize arrays as well as records.
On Thu, 17 Sep 2015, Alan Lawrence wrote:
> On 15/09/15 08:43, Richard Biener wrote:
> >
> > Sorry for chiming in so late...
>
> Not at all, TYVM for your help!
>
> > TREE_CONSTANT isn't the correct thing to test. You should use
> > TREE_CODE () == INTEGER_CST instead.
>
> Done (in some cases, via tree_fits_shwi_p).
>
> > Also you need to handle
> > NULL_TREE TYPE_MIN/MAX_VALUE as that can happen as well.
>
> I've not found any documentation as to what these mean, but from experiment,
> it seems that a zero-length array has MIN_VALUE 0 and MAX_VALUE null (as well
> as zero TYPE_SIZE) - so I allow that. In contrast a variable-length array also
> has zero TYPE_SIZE, but a large range of MIN-MAX, and I think I want to rule
> those out.
>
> Another awkward case is Ada arrays with large offset (e.g.
> array(2^32...2^32+1)
> which has only two elements); I don't see either of tree_to_shwi or
> tree_to_uhwi
> as necessarily being "better" here, each will handle (some) (rare) cases the
> other will not, so I've tried to use tree_to_shwi throughout for consistency.
>
> Summary: taken advantage of tree_fits_shwi_p, as this includes a check against
> NULL_TREE and that TREE_CODE () == INTEGER_CST.
>
> > + if (DECL_P (elem) && DECL_BIT_FIELD (elem))
> > + return false;
> >
> > that can't happen (TREE_TYPE (array-type) is never a DECL).
>
> Removed.
>
> > + int el_size = tree_to_uhwi (elem_size);
> > + gcc_assert (el_size);
> >
> > so you assert on el_size being > 0 later but above you test
> > only array size ...
>
> Good point, thanks.
>
> > + tree t_idx = build_int_cst (TYPE_DOMAIN (decl_type), idx);
> >
> > use t_idx = size_int (idx);
>
> Done.
>
> I've also added another test, of scalarizing a structure containing a
> zero-length array, as earlier attempts accidentally prevented this.
>
> Bootstrapped + check-gcc,g++,ada,fortran on ARM and x86_64;
> Bootstrapped + check-gcc,g++,fortran on AArch64.
>
> OK for trunk?
Ok.
Thanks,
Richard.
> Thanks,
> Alan
>
> gcc/ChangeLog:
>
> PR tree-optimization/67283
> * tree-sra.c (type_consists_of_records_p): Rename to...
> (scalarizable_type_p): ...this, add case for ARRAY_TYPE.
> (completely_scalarize_record): Rename to...
> (completely_scalarize): ...this, add ARRAY_TYPE case, move some code to:
> (scalarize_elem): New.
> (analyze_all_variable_accesses): Follow renamings.
>
> gcc/testsuite/ChangeLog:
>
> PR tree-optimization/67283
> * gcc.dg/tree-ssa/sra-15.c: New.
> * gcc.dg/tree-ssa/sra-16.c: New.
> ---
> gcc/testsuite/gcc.dg/tree-ssa/sra-15.c | 37
> gcc/testsuite/gcc.dg/tree-ssa/sra-16.c | 37
> gcc/tree-sra.c | 165
> +++--
> 3 files changed, 191 insertions(+), 48 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/sra-16.c
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
> b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
> new file mode 100644
> index 000..a22062e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
> @@ -0,0 +1,37 @@
> +/* Verify that SRA total scalarization works on records containing arrays.
> */
> +/* { dg-do run } */
> +/* { dg-options "-O1 -fdump-tree-release_ssa --param
> sra-max-scalarization-size-Ospeed=32" } */
> +
> +extern void abort (void);
> +
> +struct S
> +{
> + char c;
> + unsigned short f[2][2];
> + int i;
> + unsigned short f3, f4;
> +};
> +
> +
> +int __attribute__ ((noinline))
> +foo (struct S *p)
> +{
> + struct S l;
> +
> + l = *p;
> + l.i++;
> + l.f[1][0] += 3;
> + *p = l;
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> + struct S a = {0, { {5, 7}, {9, 11} }, 4, 0, 0};
> + foo (&a);
> + if (a.i != 5 || a.f[1][0] != 12)
> +abort ();
> + return 0;
> +}
> +
> +/* { dg-final { scan-tree-dump-times "l;" 0 "release_ssa" } } */
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/sra-16.c
> b/gcc/testsuite/gcc.dg/tree-ssa/sra-16.c
> new file mode 100644
> index 000..fef34c0
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-16.c
> @@ -0,0 +1,37 @@
> +/* Verify that SRA total scalarization works on records containing arrays.
> */
> +/* { dg-do run } */
> +/* { dg-options "-O1 -fdump-tree-release_ssa --param
> sra-max-scalarization-size-Ospeed=16" } */
> +
> +extern void abort (void);
> +
> +struct S
> +{
> + long zilch[0];
> + char c;
> + int i;
> + unsigned short f3, f4;
> +};
> +
> +
> +int __attribute__ ((noinline))
> +foo (struct S *p)
> +{
> + struct S l;
> +
> + l = *p;
> + l.i++;
> + l.f3++;
> + *p = l;
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> + struct S a = { { }, 0, 4, 0, 0};
> + foo (&a);
> + if (a.i != 5 || a.f3 != 1)
> +abort ();
> + return 0;
> +}
> +
> +/* { dg-final { scan-tree-dump-times "l;" 0 "release_ssa" } } */
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index 8b3a0ad..8
Re: [PATCH 2/5] completely_scalarize arrays as well as records.
On 15/09/15 08:43, Richard Biener wrote:
>
> Sorry for chiming in so late...
Not at all, TYVM for your help!
> TREE_CONSTANT isn't the correct thing to test. You should use
> TREE_CODE () == INTEGER_CST instead.
Done (in some cases, via tree_fits_shwi_p).
> Also you need to handle
> NULL_TREE TYPE_MIN/MAX_VALUE as that can happen as well.
I've not found any documentation as to what these mean, but from experiment,
it seems that a zero-length array has MIN_VALUE 0 and MAX_VALUE null (as well
as zero TYPE_SIZE) - so I allow that. In contrast a variable-length array also
has zero TYPE_SIZE, but a large range of MIN-MAX, and I think I want to rule
those out.
Another awkward case is Ada arrays with large offset (e.g. array(2^32...2^32+1)
which has only two elements); I don't see either of tree_to_shwi or tree_to_uhwi
as necessarily being "better" here, each will handle (some) (rare) cases the
other will not, so I've tried to use tree_to_shwi throughout for consistency.
Summary: taken advantage of tree_fits_shwi_p, as this includes a check against
NULL_TREE and that TREE_CODE () == INTEGER_CST.
> + if (DECL_P (elem) && DECL_BIT_FIELD (elem))
> + return false;
>
> that can't happen (TREE_TYPE (array-type) is never a DECL).
Removed.
> + int el_size = tree_to_uhwi (elem_size);
> + gcc_assert (el_size);
>
> so you assert on el_size being > 0 later but above you test
> only array size ...
Good point, thanks.
> + tree t_idx = build_int_cst (TYPE_DOMAIN (decl_type), idx);
>
> use t_idx = size_int (idx);
Done.
I've also added another test, of scalarizing a structure containing a
zero-length array, as earlier attempts accidentally prevented this.
Bootstrapped + check-gcc,g++,ada,fortran on ARM and x86_64;
Bootstrapped + check-gcc,g++,fortran on AArch64.
OK for trunk?
Thanks,
Alan
gcc/ChangeLog:
PR tree-optimization/67283
* tree-sra.c (type_consists_of_records_p): Rename to...
(scalarizable_type_p): ...this, add case for ARRAY_TYPE.
(completely_scalarize_record): Rename to...
(completely_scalarize): ...this, add ARRAY_TYPE case, move some code to:
(scalarize_elem): New.
(analyze_all_variable_accesses): Follow renamings.
gcc/testsuite/ChangeLog:
PR tree-optimization/67283
* gcc.dg/tree-ssa/sra-15.c: New.
* gcc.dg/tree-ssa/sra-16.c: New.
---
gcc/testsuite/gcc.dg/tree-ssa/sra-15.c | 37
gcc/testsuite/gcc.dg/tree-ssa/sra-16.c | 37
gcc/tree-sra.c | 165 +++--
3 files changed, 191 insertions(+), 48 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/sra-16.c
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
new file mode 100644
index 000..a22062e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
@@ -0,0 +1,37 @@
+/* Verify that SRA total scalarization works on records containing arrays. */
+/* { dg-do run } */
+/* { dg-options "-O1 -fdump-tree-release_ssa --param
sra-max-scalarization-size-Ospeed=32" } */
+
+extern void abort (void);
+
+struct S
+{
+ char c;
+ unsigned short f[2][2];
+ int i;
+ unsigned short f3, f4;
+};
+
+
+int __attribute__ ((noinline))
+foo (struct S *p)
+{
+ struct S l;
+
+ l = *p;
+ l.i++;
+ l.f[1][0] += 3;
+ *p = l;
+}
+
+int
+main (int argc, char **argv)
+{
+ struct S a = {0, { {5, 7}, {9, 11} }, 4, 0, 0};
+ foo (&a);
+ if (a.i != 5 || a.f[1][0] != 12)
+abort ();
+ return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "l;" 0 "release_ssa" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/sra-16.c
b/gcc/testsuite/gcc.dg/tree-ssa/sra-16.c
new file mode 100644
index 000..fef34c0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-16.c
@@ -0,0 +1,37 @@
+/* Verify that SRA total scalarization works on records containing arrays. */
+/* { dg-do run } */
+/* { dg-options "-O1 -fdump-tree-release_ssa --param
sra-max-scalarization-size-Ospeed=16" } */
+
+extern void abort (void);
+
+struct S
+{
+ long zilch[0];
+ char c;
+ int i;
+ unsigned short f3, f4;
+};
+
+
+int __attribute__ ((noinline))
+foo (struct S *p)
+{
+ struct S l;
+
+ l = *p;
+ l.i++;
+ l.f3++;
+ *p = l;
+}
+
+int
+main (int argc, char **argv)
+{
+ struct S a = { { }, 0, 4, 0, 0};
+ foo (&a);
+ if (a.i != 5 || a.f3 != 1)
+abort ();
+ return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "l;" 0 "release_ssa" } } */
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 8b3a0ad..8247554 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -915,73 +915,142 @@ create_access (tree expr, gimple stmt, bool write)
}
-/* Return true iff TYPE is a RECORD_TYPE with fields that are either of gimple
- register types or (recursively) records with only these two kinds of fields.
- It also returns false if any of these records contains a bit-field. */
+/* Return true iff TYPE is scalari
Re: [PATCH 2/5] completely_scalarize arrays as well as records.
On Mon, 14 Sep 2015, Alan Lawrence wrote: > Ping. (Rerevert with 5 lines extra paranoia in scalarizable_type_p). Sorry for chiming in so late... + if (TYPE_DOMAIN (type) == NULL_TREE + || !TREE_CONSTANT (TYPE_MIN_VALUE (TYPE_DOMAIN (type))) + || !TREE_CONSTANT (TYPE_MAX_VALUE (TYPE_DOMAIN (type))) + || !TREE_CONSTANT (TYPE_SIZE (type)) + || (tree_to_shwi (TYPE_SIZE (type)) <= 0)) + return false; TREE_CONSTANT isn't the correct thing to test. You should use TREE_CODE () == INTEGER_CST instead. Also you need to handle NULL_TREE TYPE_MIN/MAX_VALUE as that can happen as well. + if (DECL_P (elem) && DECL_BIT_FIELD (elem)) + return false; that can't happen (TREE_TYPE (array-type) is never a DECL). + int el_size = tree_to_uhwi (elem_size); + gcc_assert (el_size); so you assert on el_size being > 0 later but above you test only array size ... + tree t_idx = build_int_cst (TYPE_DOMAIN (decl_type), idx); use t_idx = size_int (idx); + tree nref = build4 (ARRAY_REF, elemtype, ref, t_idx, NULL_TREE, + NULL_TREE); Otherwise looks ok to me. Thanks, Richard. > Thanks, Alan > > On 08/09/15 13:43, Martin Jambor wrote: > > Hi, > > > > On Mon, Sep 07, 2015 at 02:15:45PM +0100, Alan Lawrence wrote: > > > In-Reply-To: <55e0697d.2010...@arm.com> > > > > > > On 28/08/15 16:08, Alan Lawrence wrote: > > > > Alan Lawrence wrote: > > > > > > > > > > Right. I think VLA's are the problem with pr64312.C also. I'm testing > > > > > a fix > > > > > (that declares arrays with any of these properties as unscalarizable). > > > > ... > > > > In the meantime I've reverted the patch pending further testing on x86, > > > > aarch64 > > > > and arm. > > > > > > I've now tested g++ and fortran (+ bootstrap + check-gcc) on x86, AArch64 > > > and > > > ARM, and Ada on x86 and ARM. > > > > > > So far the list of failures from the original patch seems to be: > > > > > > * g++.dg/torture/pr64312.C on ARM and m68k-linux > > > * Building Ada on x86 > > > * Ada ACATS c87b31a on ARM (where the Ada frontend builds fine) > > > > > > Here's a new version, that fixes all the above, by adding a dose of > > > paranoia in scalarizable_type_p... > > > > I have only had a bref look at scalarizable_type_p then, considering > > all of the rest unchanged, and the tests there seem natural to me. > > (Note that I do not have the authority to approve the patch.) > > > > > (I wonder about adding a comment > > > in completely_scalarize that such cases have already been ruled > > > out?) > > > > The comment already references scalarizable_type_p which is enough at > > least for me. > > > > Thanks, > > > > Martin > > > > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Re: [PATCH 2/5] completely_scalarize arrays as well as records.
Ping. (Rerevert with 5 lines extra paranoia in scalarizable_type_p). Thanks, Alan On 08/09/15 13:43, Martin Jambor wrote: Hi, On Mon, Sep 07, 2015 at 02:15:45PM +0100, Alan Lawrence wrote: In-Reply-To: <55e0697d.2010...@arm.com> On 28/08/15 16:08, Alan Lawrence wrote: Alan Lawrence wrote: Right. I think VLA's are the problem with pr64312.C also. I'm testing a fix (that declares arrays with any of these properties as unscalarizable). ... In the meantime I've reverted the patch pending further testing on x86, aarch64 and arm. I've now tested g++ and fortran (+ bootstrap + check-gcc) on x86, AArch64 and ARM, and Ada on x86 and ARM. So far the list of failures from the original patch seems to be: * g++.dg/torture/pr64312.C on ARM and m68k-linux * Building Ada on x86 * Ada ACATS c87b31a on ARM (where the Ada frontend builds fine) Here's a new version, that fixes all the above, by adding a dose of paranoia in scalarizable_type_p... I have only had a bref look at scalarizable_type_p then, considering all of the rest unchanged, and the tests there seem natural to me. (Note that I do not have the authority to approve the patch.) (I wonder about adding a comment in completely_scalarize that such cases have already been ruled out?) The comment already references scalarizable_type_p which is enough at least for me. Thanks, Martin
Re: [PATCH 2/5] completely_scalarize arrays as well as records.
Hi, On Mon, Sep 07, 2015 at 02:15:45PM +0100, Alan Lawrence wrote: > In-Reply-To: <55e0697d.2010...@arm.com> > > On 28/08/15 16:08, Alan Lawrence wrote: > > Alan Lawrence wrote: > >> > >> Right. I think VLA's are the problem with pr64312.C also. I'm testing a fix > >> (that declares arrays with any of these properties as unscalarizable). > > ... > > In the meantime I've reverted the patch pending further testing on x86, > > aarch64 > > and arm. > > I've now tested g++ and fortran (+ bootstrap + check-gcc) on x86, AArch64 and > ARM, and Ada on x86 and ARM. > > So far the list of failures from the original patch seems to be: > > * g++.dg/torture/pr64312.C on ARM and m68k-linux > * Building Ada on x86 > * Ada ACATS c87b31a on ARM (where the Ada frontend builds fine) > > Here's a new version, that fixes all the above, by adding a dose of > paranoia in scalarizable_type_p... I have only had a bref look at scalarizable_type_p then, considering all of the rest unchanged, and the tests there seem natural to me. (Note that I do not have the authority to approve the patch.) > (I wonder about adding a comment > in completely_scalarize that such cases have already been ruled > out?) The comment already references scalarizable_type_p which is enough at least for me. Thanks, Martin
Re: [PATCH 2/5] completely_scalarize arrays as well as records.
In-Reply-To: <55e0697d.2010...@arm.com>
On 28/08/15 16:08, Alan Lawrence wrote:
> Alan Lawrence wrote:
>>
>> Right. I think VLA's are the problem with pr64312.C also. I'm testing a fix
>> (that declares arrays with any of these properties as unscalarizable).
> ...
> In the meantime I've reverted the patch pending further testing on x86,
> aarch64
> and arm.
I've now tested g++ and fortran (+ bootstrap + check-gcc) on x86, AArch64 and
ARM, and Ada on x86 and ARM.
So far the list of failures from the original patch seems to be:
* g++.dg/torture/pr64312.C on ARM and m68k-linux
* Building Ada on x86
* Ada ACATS c87b31a on ARM (where the Ada frontend builds fine)
Here's a new version, that fixes all the above, by adding a dose of paranoia in
scalarizable_type_p... (I wonder about adding a comment in completely_scalarize
that such cases have already been ruled out?)
OK to install?
Cheers, Alan
---
gcc/testsuite/gcc.dg/tree-ssa/sra-15.c | 37
gcc/tree-sra.c | 155 +++--
2 files changed, 144 insertions(+), 48 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
new file mode 100644
index 000..a22062e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
@@ -0,0 +1,37 @@
+/* Verify that SRA total scalarization works on records containing arrays. */
+/* { dg-do run } */
+/* { dg-options "-O1 -fdump-tree-release_ssa --param
sra-max-scalarization-size-Ospeed=32" } */
+
+extern void abort (void);
+
+struct S
+{
+ char c;
+ unsigned short f[2][2];
+ int i;
+ unsigned short f3, f4;
+};
+
+
+int __attribute__ ((noinline))
+foo (struct S *p)
+{
+ struct S l;
+
+ l = *p;
+ l.i++;
+ l.f[1][0] += 3;
+ *p = l;
+}
+
+int
+main (int argc, char **argv)
+{
+ struct S a = {0, { {5, 7}, {9, 11} }, 4, 0, 0};
+ foo (&a);
+ if (a.i != 5 || a.f[1][0] != 12)
+abort ();
+ return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "l;" 0 "release_ssa" } } */
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 8b3a0ad..d9fe058 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -915,73 +915,132 @@ create_access (tree expr, gimple stmt, bool write)
}
-/* Return true iff TYPE is a RECORD_TYPE with fields that are either of gimple
- register types or (recursively) records with only these two kinds of fields.
- It also returns false if any of these records contains a bit-field. */
+/* Return true iff TYPE is scalarizable - i.e. a RECORD_TYPE or fixed-length
+ ARRAY_TYPE with fields that are either of gimple register types (excluding
+ bit-fields) or (recursively) scalarizable types. */
static bool
-type_consists_of_records_p (tree type)
+scalarizable_type_p (tree type)
{
- tree fld;
+ gcc_assert (!is_gimple_reg_type (type));
- if (TREE_CODE (type) != RECORD_TYPE)
-return false;
+ switch (TREE_CODE (type))
+ {
+ case RECORD_TYPE:
+for (tree fld = TYPE_FIELDS (type); fld; fld = DECL_CHAIN (fld))
+ if (TREE_CODE (fld) == FIELD_DECL)
+ {
+ tree ft = TREE_TYPE (fld);
- for (fld = TYPE_FIELDS (type); fld; fld = DECL_CHAIN (fld))
-if (TREE_CODE (fld) == FIELD_DECL)
- {
- tree ft = TREE_TYPE (fld);
+ if (DECL_BIT_FIELD (fld))
+ return false;
- if (DECL_BIT_FIELD (fld))
- return false;
+ if (!is_gimple_reg_type (ft)
+ && !scalarizable_type_p (ft))
+ return false;
+ }
- if (!is_gimple_reg_type (ft)
- && !type_consists_of_records_p (ft))
- return false;
- }
+return true;
- return true;
+ case ARRAY_TYPE:
+{
+ if (TYPE_DOMAIN (type) == NULL_TREE
+ || !TREE_CONSTANT (TYPE_MIN_VALUE (TYPE_DOMAIN (type)))
+ || !TREE_CONSTANT (TYPE_MAX_VALUE (TYPE_DOMAIN (type)))
+ || !TREE_CONSTANT (TYPE_SIZE (type))
+ || (tree_to_shwi (TYPE_SIZE (type)) <= 0))
+ return false;
+ tree elem = TREE_TYPE (type);
+ if (DECL_P (elem) && DECL_BIT_FIELD (elem))
+ return false;
+ if (!is_gimple_reg_type (elem)
+&& !scalarizable_type_p (elem))
+ return false;
+ return true;
+}
+ default:
+return false;
+ }
}
-/* Create total_scalarization accesses for all scalar type fields in DECL that
- must be of a RECORD_TYPE conforming to type_consists_of_records_p. BASE
- must be the top-most VAR_DECL representing the variable, OFFSET must be the
- offset of DECL within BASE. REF must be the memory reference expression for
- the given decl. */
+static void scalarize_elem (tree, HOST_WIDE_INT, HOST_WIDE_INT, tree, tree);
+
+/* Create total_scalarization accesses for all scalar fields of a member
+ of type DECL_TYPE conforming to scalarizable_type_p. BASE
+ must be the top-most VAR_DECL representing the variable; within that,
+ OFFSET locates the member and REF must be the memory reference expression
Re: [PATCH 2/5] completely_scalarize arrays as well as records
Alan Lawrence wrote: Right. I think VLA's are the problem with pr64312.C also. I'm testing a fix (that declares arrays with any of these properties as unscalarizable). Monday is a bank holiday in UK and so I expect to get back to you on Tuesday. --Alan In the meantime I've reverted the patch pending further testing on x86, aarch64 and arm. --Alan
Re: [PATCH 2/5] completely_scalarize arrays as well as records
Richard Biener wrote:
On Fri, 28 Aug 2015, Alan Lawrence wrote:
Christophe Lyon wrote:
I asked because I assumed that Alan saw it pass in his configuration.
Bah. No - I now discover a problem in my C++ testsuite setup that was causing
a large number of tests to not be executed. I see the problem too now,
investigating
Btw, your patch broke Ada:
+===GNAT BUG
DETECTED==+
| 6.0.0 20150828 (experimental) (x86_64-pc-linux-gnu) GCC error:
|
| in completely_scalarize, at tree-sra.c:996
|
| Error detected around ../rts/a-coorse.ads:46:24
|
case ARRAY_TYPE:
{
tree elemtype = TREE_TYPE (decl_type);
tree elem_size = TYPE_SIZE (elemtype);
gcc_assert (elem_size && tree_fits_uhwi_p (elem_size));
int el_size = tree_to_uhwi (elem_size);
gcc_assert (el_size);
tree minidx = TYPE_MIN_VALUE (TYPE_DOMAIN (decl_type));
tree maxidx = TYPE_MAX_VALUE (TYPE_DOMAIN (decl_type));
gcc_assert (TREE_CODE (minidx) == INTEGER_CST
&& TREE_CODE (maxidx) == INTEGER_CST);
obviously you missed VLAs. min/max value can also be NULL.
Richard.
Right. I think VLA's are the problem with pr64312.C also. I'm testing a fix
(that declares arrays with any of these properties as unscalarizable).
Monday is a bank holiday in UK and so I expect to get back to you on Tuesday.
--Alan
Re: [PATCH 2/5] completely_scalarize arrays as well as records
On Fri, 28 Aug 2015, Alan Lawrence wrote:
> Christophe Lyon wrote:
> >
> > I asked because I assumed that Alan saw it pass in his configuration.
>
>
> Bah. No - I now discover a problem in my C++ testsuite setup that was causing
> a large number of tests to not be executed. I see the problem too now,
> investigating
Btw, your patch broke Ada:
+===GNAT BUG
DETECTED==+
| 6.0.0 20150828 (experimental) (x86_64-pc-linux-gnu) GCC error:
|
| in completely_scalarize, at tree-sra.c:996
|
| Error detected around ../rts/a-coorse.ads:46:24
|
case ARRAY_TYPE:
{
tree elemtype = TREE_TYPE (decl_type);
tree elem_size = TYPE_SIZE (elemtype);
gcc_assert (elem_size && tree_fits_uhwi_p (elem_size));
int el_size = tree_to_uhwi (elem_size);
gcc_assert (el_size);
tree minidx = TYPE_MIN_VALUE (TYPE_DOMAIN (decl_type));
tree maxidx = TYPE_MAX_VALUE (TYPE_DOMAIN (decl_type));
gcc_assert (TREE_CODE (minidx) == INTEGER_CST
&& TREE_CODE (maxidx) == INTEGER_CST);
obviously you missed VLAs. min/max value can also be NULL.
Richard.
Re: [PATCH 2/5] completely_scalarize arrays as well as records
Christophe Lyon wrote: I asked because I assumed that Alan saw it pass in his configuration. Bah. No - I now discover a problem in my C++ testsuite setup that was causing a large number of tests to not be executed. I see the problem too now, investigating --Alan
Re: [PATCH 2/5] completely_scalarize arrays as well as records
On Fri, 28 Aug 2015, Christophe Lyon wrote: > On 28 August 2015 at 09:48, Richard Biener wrote: > > On Fri, 28 Aug 2015, Christophe Lyon wrote: > > > >> On 27 August 2015 at 17:43, Alan Lawrence wrote: > >> > Martin Jambor wrote: > >> >> > >> >> First, I would be much > >> >> happier if you added a proper comment to scalarize_elem function which > >> >> you forgot completely. The name is not very descriptive and it has > >> >> quite few parameters too. > >> >> > >> >> Second, this patch should also fix PR 67283. It would be great if you > >> >> could verify that and add it to the changelog when committing if that > >> >> is indeed the case. > >> > > >> > Thanks for pointing both of those out. I've added a comment to > >> > scalarize_elem, > >> > deleted the bogus comment in the new test, and yes I can confirm that > >> > the patch > >> > fixes PR 67283 on x86_64, and also AArch64 if > >> > --param sra-max-scalarization-size-Ospeed is passed. (I've not added any > >> > testcase specifically taken from that PR, however.) > >> > > >> > Pushed as r277265. > >> > >> Actually, is r227265. > >> > >> Since since commit I've noticed that > >> g++.dg/torture/pr64312.C > >> fails at -O1 in my config, saying "virtual memory exhaustion" (arm* > >> targets) > >> I run my validations under ulimit -v 10GB, which seems already large > >> enough. > >> > >> Do we consider this a bug? > > > > Sure we do. You have to investigate this (I guess we run into some > > endless looping/recursing that eats memory somewhere). > > > > I asked because I assumed that Alan saw it pass in his configuration. Well, it should still be investigated - whether you caused it or not ;) It's a bug. Richard.
Re: [PATCH 2/5] completely_scalarize arrays as well as records
On 28 August 2015 at 09:48, Richard Biener wrote: > On Fri, 28 Aug 2015, Christophe Lyon wrote: > >> On 27 August 2015 at 17:43, Alan Lawrence wrote: >> > Martin Jambor wrote: >> >> >> >> First, I would be much >> >> happier if you added a proper comment to scalarize_elem function which >> >> you forgot completely. The name is not very descriptive and it has >> >> quite few parameters too. >> >> >> >> Second, this patch should also fix PR 67283. It would be great if you >> >> could verify that and add it to the changelog when committing if that >> >> is indeed the case. >> > >> > Thanks for pointing both of those out. I've added a comment to >> > scalarize_elem, >> > deleted the bogus comment in the new test, and yes I can confirm that the >> > patch >> > fixes PR 67283 on x86_64, and also AArch64 if >> > --param sra-max-scalarization-size-Ospeed is passed. (I've not added any >> > testcase specifically taken from that PR, however.) >> > >> > Pushed as r277265. >> >> Actually, is r227265. >> >> Since since commit I've noticed that >> g++.dg/torture/pr64312.C >> fails at -O1 in my config, saying "virtual memory exhaustion" (arm* targets) >> I run my validations under ulimit -v 10GB, which seems already large enough. >> >> Do we consider this a bug? > > Sure we do. You have to investigate this (I guess we run into some > endless looping/recursing that eats memory somewhere). > I asked because I assumed that Alan saw it pass in his configuration. > Richard.
Re: [PATCH 2/5] completely_scalarize arrays as well as records
On Fri, 28 Aug 2015, Christophe Lyon wrote: > On 27 August 2015 at 17:43, Alan Lawrence wrote: > > Martin Jambor wrote: > >> > >> First, I would be much > >> happier if you added a proper comment to scalarize_elem function which > >> you forgot completely. The name is not very descriptive and it has > >> quite few parameters too. > >> > >> Second, this patch should also fix PR 67283. It would be great if you > >> could verify that and add it to the changelog when committing if that > >> is indeed the case. > > > > Thanks for pointing both of those out. I've added a comment to > > scalarize_elem, > > deleted the bogus comment in the new test, and yes I can confirm that the > > patch > > fixes PR 67283 on x86_64, and also AArch64 if > > --param sra-max-scalarization-size-Ospeed is passed. (I've not added any > > testcase specifically taken from that PR, however.) > > > > Pushed as r277265. > > Actually, is r227265. > > Since since commit I've noticed that > g++.dg/torture/pr64312.C > fails at -O1 in my config, saying "virtual memory exhaustion" (arm* targets) > I run my validations under ulimit -v 10GB, which seems already large enough. > > Do we consider this a bug? Sure we do. You have to investigate this (I guess we run into some endless looping/recursing that eats memory somewhere). Richard.
Re: [PATCH 2/5] completely_scalarize arrays as well as records
On 27 August 2015 at 17:43, Alan Lawrence wrote:
> Martin Jambor wrote:
>>
>> First, I would be much
>> happier if you added a proper comment to scalarize_elem function which
>> you forgot completely. The name is not very descriptive and it has
>> quite few parameters too.
>>
>> Second, this patch should also fix PR 67283. It would be great if you
>> could verify that and add it to the changelog when committing if that
>> is indeed the case.
>
> Thanks for pointing both of those out. I've added a comment to scalarize_elem,
> deleted the bogus comment in the new test, and yes I can confirm that the
> patch
> fixes PR 67283 on x86_64, and also AArch64 if
> --param sra-max-scalarization-size-Ospeed is passed. (I've not added any
> testcase specifically taken from that PR, however.)
>
> Pushed as r277265.
Actually, is r227265.
Since since commit I've noticed that
g++.dg/torture/pr64312.C
fails at -O1 in my config, saying "virtual memory exhaustion" (arm* targets)
I run my validations under ulimit -v 10GB, which seems already large enough.
Do we consider this a bug?
Christophe.
> ---
> gcc/testsuite/gcc.dg/tree-ssa/sra-15.c | 37
> gcc/tree-sra.c | 149
> ++---
> 2 files changed, 138 insertions(+), 48 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
> b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
> new file mode 100644
> index 000..a22062e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
> @@ -0,0 +1,37 @@
> +/* Verify that SRA total scalarization works on records containing arrays.
> */
> +/* { dg-do run } */
> +/* { dg-options "-O1 -fdump-tree-release_ssa --param
> sra-max-scalarization-size-Ospeed=32" } */
> +
> +extern void abort (void);
> +
> +struct S
> +{
> + char c;
> + unsigned short f[2][2];
> + int i;
> + unsigned short f3, f4;
> +};
> +
> +
> +int __attribute__ ((noinline))
> +foo (struct S *p)
> +{
> + struct S l;
> +
> + l = *p;
> + l.i++;
> + l.f[1][0] += 3;
> + *p = l;
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> + struct S a = {0, { {5, 7}, {9, 11} }, 4, 0, 0};
> + foo (&a);
> + if (a.i != 5 || a.f[1][0] != 12)
> +abort ();
> + return 0;
> +}
> +
> +/* { dg-final { scan-tree-dump-times "l;" 0 "release_ssa" } } */
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index 8b3a0ad..3caf84a 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -915,73 +915,126 @@ create_access (tree expr, gimple stmt, bool write)
> }
>
>
> -/* Return true iff TYPE is a RECORD_TYPE with fields that are either of
> gimple
> - register types or (recursively) records with only these two kinds of
> fields.
> - It also returns false if any of these records contains a bit-field. */
> +/* Return true iff TYPE is scalarizable - i.e. a RECORD_TYPE or ARRAY_TYPE
> with
> + fields that are either of gimple register types (excluding bit-fields)
> + or (recursively) scalarizable types. */
>
> static bool
> -type_consists_of_records_p (tree type)
> +scalarizable_type_p (tree type)
> {
> - tree fld;
> + gcc_assert (!is_gimple_reg_type (type));
>
> - if (TREE_CODE (type) != RECORD_TYPE)
> -return false;
> + switch (TREE_CODE (type))
> + {
> + case RECORD_TYPE:
> +for (tree fld = TYPE_FIELDS (type); fld; fld = DECL_CHAIN (fld))
> + if (TREE_CODE (fld) == FIELD_DECL)
> + {
> + tree ft = TREE_TYPE (fld);
>
> - for (fld = TYPE_FIELDS (type); fld; fld = DECL_CHAIN (fld))
> -if (TREE_CODE (fld) == FIELD_DECL)
> - {
> - tree ft = TREE_TYPE (fld);
> + if (DECL_BIT_FIELD (fld))
> + return false;
>
> - if (DECL_BIT_FIELD (fld))
> - return false;
> + if (!is_gimple_reg_type (ft)
> + && !scalarizable_type_p (ft))
> + return false;
> + }
>
> - if (!is_gimple_reg_type (ft)
> - && !type_consists_of_records_p (ft))
> - return false;
> - }
> +return true;
>
> - return true;
> + case ARRAY_TYPE:
> +{
> + tree elem = TREE_TYPE (type);
> + if (DECL_P (elem) && DECL_BIT_FIELD (elem))
> + return false;
> + if (!is_gimple_reg_type (elem)
> +&& !scalarizable_type_p (elem))
> + return false;
> + return true;
> +}
> + default:
> +return false;
> + }
> }
>
> -/* Create total_scalarization accesses for all scalar type fields in DECL
> that
> - must be of a RECORD_TYPE conforming to type_consists_of_records_p. BASE
> - must be the top-most VAR_DECL representing the variable, OFFSET must be
> the
> - offset of DECL within BASE. REF must be the memory reference expression
> for
> - the given decl. */
> +static void scalarize_elem (tree, HOST_WIDE_INT, HOST_WIDE_INT, tree, tree);
> +
> +/* Create total_scalarization accesses for all scalar fields of a member
> + of type DECL_TYPE conforming to scalarizable_type_p. BASE
> + must b
Fixing sra-12.c (was: Re: [PATCH 2/5] completely_scalarize arrays as well as records)
Jeff Law wrote: diff --git a/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c new file mode 100644 index 000..e251058 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c @@ -0,0 +1,38 @@ +/* Verify that SRA total scalarization works on records containing arrays. */ +/* Test skipped for targets with small (often default) MOVE_RATIO. */ ?!? I don't see anything that skips this test for any targets. Presumably this was copied from sra-12.c. I suspect this comment should just be removed. You are right, thank you. Copied from sra-12.c, was I that obvious? ;). Indeed I notice that the same trick (of passing --param sra-max-scalarization-size-Ospeed=32) enables sra-12.c to pass on avr-unknown-linux-gnu and sh-unknown-linux-gnu, too; I haven't managed to build a compiler for nds32 yet... --Alan
Re: [PATCH 2/5] completely_scalarize arrays as well as records
Martin Jambor wrote:
>
> First, I would be much
> happier if you added a proper comment to scalarize_elem function which
> you forgot completely. The name is not very descriptive and it has
> quite few parameters too.
>
> Second, this patch should also fix PR 67283. It would be great if you
> could verify that and add it to the changelog when committing if that
> is indeed the case.
Thanks for pointing both of those out. I've added a comment to scalarize_elem,
deleted the bogus comment in the new test, and yes I can confirm that the patch
fixes PR 67283 on x86_64, and also AArch64 if
--param sra-max-scalarization-size-Ospeed is passed. (I've not added any
testcase specifically taken from that PR, however.)
Pushed as r277265.
---
gcc/testsuite/gcc.dg/tree-ssa/sra-15.c | 37
gcc/tree-sra.c | 149 ++---
2 files changed, 138 insertions(+), 48 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
new file mode 100644
index 000..a22062e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
@@ -0,0 +1,37 @@
+/* Verify that SRA total scalarization works on records containing arrays. */
+/* { dg-do run } */
+/* { dg-options "-O1 -fdump-tree-release_ssa --param
sra-max-scalarization-size-Ospeed=32" } */
+
+extern void abort (void);
+
+struct S
+{
+ char c;
+ unsigned short f[2][2];
+ int i;
+ unsigned short f3, f4;
+};
+
+
+int __attribute__ ((noinline))
+foo (struct S *p)
+{
+ struct S l;
+
+ l = *p;
+ l.i++;
+ l.f[1][0] += 3;
+ *p = l;
+}
+
+int
+main (int argc, char **argv)
+{
+ struct S a = {0, { {5, 7}, {9, 11} }, 4, 0, 0};
+ foo (&a);
+ if (a.i != 5 || a.f[1][0] != 12)
+abort ();
+ return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "l;" 0 "release_ssa" } } */
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 8b3a0ad..3caf84a 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -915,73 +915,126 @@ create_access (tree expr, gimple stmt, bool write)
}
-/* Return true iff TYPE is a RECORD_TYPE with fields that are either of gimple
- register types or (recursively) records with only these two kinds of fields.
- It also returns false if any of these records contains a bit-field. */
+/* Return true iff TYPE is scalarizable - i.e. a RECORD_TYPE or ARRAY_TYPE with
+ fields that are either of gimple register types (excluding bit-fields)
+ or (recursively) scalarizable types. */
static bool
-type_consists_of_records_p (tree type)
+scalarizable_type_p (tree type)
{
- tree fld;
+ gcc_assert (!is_gimple_reg_type (type));
- if (TREE_CODE (type) != RECORD_TYPE)
-return false;
+ switch (TREE_CODE (type))
+ {
+ case RECORD_TYPE:
+for (tree fld = TYPE_FIELDS (type); fld; fld = DECL_CHAIN (fld))
+ if (TREE_CODE (fld) == FIELD_DECL)
+ {
+ tree ft = TREE_TYPE (fld);
- for (fld = TYPE_FIELDS (type); fld; fld = DECL_CHAIN (fld))
-if (TREE_CODE (fld) == FIELD_DECL)
- {
- tree ft = TREE_TYPE (fld);
+ if (DECL_BIT_FIELD (fld))
+ return false;
- if (DECL_BIT_FIELD (fld))
- return false;
+ if (!is_gimple_reg_type (ft)
+ && !scalarizable_type_p (ft))
+ return false;
+ }
- if (!is_gimple_reg_type (ft)
- && !type_consists_of_records_p (ft))
- return false;
- }
+return true;
- return true;
+ case ARRAY_TYPE:
+{
+ tree elem = TREE_TYPE (type);
+ if (DECL_P (elem) && DECL_BIT_FIELD (elem))
+ return false;
+ if (!is_gimple_reg_type (elem)
+&& !scalarizable_type_p (elem))
+ return false;
+ return true;
+}
+ default:
+return false;
+ }
}
-/* Create total_scalarization accesses for all scalar type fields in DECL that
- must be of a RECORD_TYPE conforming to type_consists_of_records_p. BASE
- must be the top-most VAR_DECL representing the variable, OFFSET must be the
- offset of DECL within BASE. REF must be the memory reference expression for
- the given decl. */
+static void scalarize_elem (tree, HOST_WIDE_INT, HOST_WIDE_INT, tree, tree);
+
+/* Create total_scalarization accesses for all scalar fields of a member
+ of type DECL_TYPE conforming to scalarizable_type_p. BASE
+ must be the top-most VAR_DECL representing the variable; within that,
+ OFFSET locates the member and REF must be the memory reference expression
for
+ the member. */
static void
-completely_scalarize_record (tree base, tree decl, HOST_WIDE_INT offset,
-tree ref)
+completely_scalarize (tree base, tree decl_type, HOST_WIDE_INT offset, tree
ref)
{
- tree fld, decl_type = TREE_TYPE (decl);
+ switch (TREE_CODE (decl_type))
+{
+case RECORD_TYPE:
+ for (tree fld = TYPE_FIELDS (decl_type); fld; fld = DECL_CHAIN (fld))
+ if (TREE_CODE (fld) == FIELD_DECL)
+ {
+
Re: [PATCH 2/5] completely_scalarize arrays as well as records
On August 26, 2015 6:08:55 PM GMT+02:00, Alan Lawrence wrote: >Richard Biener wrote: > One extra question is does the way we limit total scalarization >work >>> well for arrays? I suppose we have either sth like the maximum size of >an aggregate we scalarize or the maximum number of component accesses we create? >>> Only the former and that would be kept intact. It is in fact >visible >>> in the context of the last hunk of the patch. >> >> OK. IIRC the gimplification code also has the latter and also >considers zeroing the whole aggregate before initializing non-zero >fields. IMHO it makes sense to reuse some of the analysis and >classification routines it has. > >Do you mean gimplify_init_constructor? Yes, there's quite a lot of >logic there >;). That feels like a separate patch - and belonging to the >constant-handling >subseries of this series Yes. - as gimplify_init_constructor already deals >with both >record and array types, and I don't see anything there that's >specifically good >for total-scalarization of arrays? > >IOW, do you mean that to block this patch, or can it be separate (I can >address >Martin + Jeff's comments fairly quickly and independently) ? No, but I'd like this being explores with the init sub series. We don't want two places doing total scalarization of initualizers , gimplification and SRA and with different/conflicting heuristics. IMHO the gimplification total scalarization happens too early. Richard. >Cheers, Alan
Re: [PATCH 2/5] completely_scalarize arrays as well as records
Richard Biener wrote: One extra question is does the way we limit total scalarization work well for arrays? I suppose we have either sth like the maximum size of an aggregate we scalarize or the maximum number of component accesses we create? Only the former and that would be kept intact. It is in fact visible in the context of the last hunk of the patch. OK. IIRC the gimplification code also has the latter and also considers zeroing the whole aggregate before initializing non-zero fields. IMHO it makes sense to reuse some of the analysis and classification routines it has. Do you mean gimplify_init_constructor? Yes, there's quite a lot of logic there ;). That feels like a separate patch - and belonging to the constant-handling subseries of this series - as gimplify_init_constructor already deals with both record and array types, and I don't see anything there that's specifically good for total-scalarization of arrays? IOW, do you mean that to block this patch, or can it be separate (I can address Martin + Jeff's comments fairly quickly and independently) ? Cheers, Alan
Re: [PATCH 2/5] completely_scalarize arrays as well as records
On August 26, 2015 11:30:26 AM GMT+02:00, Martin Jambor wrote: >Hi, > >On Wed, Aug 26, 2015 at 09:07:33AM +0200, Richard Biener wrote: >> On Tue, Aug 25, 2015 at 11:44 PM, Jeff Law wrote: >> > On 08/25/2015 03:42 PM, Martin Jambor wrote: >> >> >> >> Hi, >> >> >> >> On Tue, Aug 25, 2015 at 12:06:14PM +0100, Alan Lawrence wrote: >> >>> >> >>> This changes the completely_scalarize_record path to also work on >arrays >> >>> (thus >> >>> allowing records containing arrays, etc.). This just required >extending >> >>> the >> >>> existing type_consists_of_records_p and >completely_scalarize_record >> >>> methods >> >>> to handle things of ARRAY_TYPE as well as RECORD_TYPE. Hence, I >renamed >> >>> both >> >>> methods so as not to mention 'record'. >> >> >> >> >> >> thanks for working on this. I see Jeff has already approved the >> >> patch, but I have two comments nevertheless. First, I would be >much >> >> happier if you added a proper comment to scalarize_elem function >which >> >> you forgot completely. The name is not very descriptive and it >has >> >> quite few parameters too. >> > >> > Right. I mentioned that I missed the lack of function comments >when looking >> > at #3 and asked Alan to go back and fix them in #1 and #2. >> > >> >> >> >> Second, this patch should also fix PR 67283. It would be great if >you >> >> could verify that and add it to the changelog when committing if >that >> >> is indeed the case. >> > >> > Excellent. Yes, definitely mention the BZ. >> >> One extra question is does the way we limit total scalarization work >well >> for arrays? I suppose we have either sth like the maximum size of an >> aggregate we scalarize or the maximum number of component accesses >> we create? >> > >Only the former and that would be kept intact. It is in fact visible >in the context of the last hunk of the patch. OK. IIRC the gimplification code also has the latter and also considers zeroing the whole aggregate before initializing non-zero fields. IMHO it makes sense to reuse some of the analysis and classification routines it has. Richard. >Martin
Re: [PATCH 2/5] completely_scalarize arrays as well as records
Hi, On Wed, Aug 26, 2015 at 09:07:33AM +0200, Richard Biener wrote: > On Tue, Aug 25, 2015 at 11:44 PM, Jeff Law wrote: > > On 08/25/2015 03:42 PM, Martin Jambor wrote: > >> > >> Hi, > >> > >> On Tue, Aug 25, 2015 at 12:06:14PM +0100, Alan Lawrence wrote: > >>> > >>> This changes the completely_scalarize_record path to also work on arrays > >>> (thus > >>> allowing records containing arrays, etc.). This just required extending > >>> the > >>> existing type_consists_of_records_p and completely_scalarize_record > >>> methods > >>> to handle things of ARRAY_TYPE as well as RECORD_TYPE. Hence, I renamed > >>> both > >>> methods so as not to mention 'record'. > >> > >> > >> thanks for working on this. I see Jeff has already approved the > >> patch, but I have two comments nevertheless. First, I would be much > >> happier if you added a proper comment to scalarize_elem function which > >> you forgot completely. The name is not very descriptive and it has > >> quite few parameters too. > > > > Right. I mentioned that I missed the lack of function comments when looking > > at #3 and asked Alan to go back and fix them in #1 and #2. > > > >> > >> Second, this patch should also fix PR 67283. It would be great if you > >> could verify that and add it to the changelog when committing if that > >> is indeed the case. > > > > Excellent. Yes, definitely mention the BZ. > > One extra question is does the way we limit total scalarization work well > for arrays? I suppose we have either sth like the maximum size of an > aggregate we scalarize or the maximum number of component accesses > we create? > Only the former and that would be kept intact. It is in fact visible in the context of the last hunk of the patch. Martin
Re: [PATCH 2/5] completely_scalarize arrays as well as records
On Tue, Aug 25, 2015 at 11:44 PM, Jeff Law wrote: > On 08/25/2015 03:42 PM, Martin Jambor wrote: >> >> Hi, >> >> On Tue, Aug 25, 2015 at 12:06:14PM +0100, Alan Lawrence wrote: >>> >>> This changes the completely_scalarize_record path to also work on arrays >>> (thus >>> allowing records containing arrays, etc.). This just required extending >>> the >>> existing type_consists_of_records_p and completely_scalarize_record >>> methods >>> to handle things of ARRAY_TYPE as well as RECORD_TYPE. Hence, I renamed >>> both >>> methods so as not to mention 'record'. >> >> >> thanks for working on this. I see Jeff has already approved the >> patch, but I have two comments nevertheless. First, I would be much >> happier if you added a proper comment to scalarize_elem function which >> you forgot completely. The name is not very descriptive and it has >> quite few parameters too. > > Right. I mentioned that I missed the lack of function comments when looking > at #3 and asked Alan to go back and fix them in #1 and #2. > >> >> Second, this patch should also fix PR 67283. It would be great if you >> could verify that and add it to the changelog when committing if that >> is indeed the case. > > Excellent. Yes, definitely mention the BZ. One extra question is does the way we limit total scalarization work well for arrays? I suppose we have either sth like the maximum size of an aggregate we scalarize or the maximum number of component accesses we create? Thanks, Richard. > jeff >
Re: [PATCH 2/5] completely_scalarize arrays as well as records
On 08/25/2015 03:42 PM, Martin Jambor wrote: Hi, On Tue, Aug 25, 2015 at 12:06:14PM +0100, Alan Lawrence wrote: This changes the completely_scalarize_record path to also work on arrays (thus allowing records containing arrays, etc.). This just required extending the existing type_consists_of_records_p and completely_scalarize_record methods to handle things of ARRAY_TYPE as well as RECORD_TYPE. Hence, I renamed both methods so as not to mention 'record'. thanks for working on this. I see Jeff has already approved the patch, but I have two comments nevertheless. First, I would be much happier if you added a proper comment to scalarize_elem function which you forgot completely. The name is not very descriptive and it has quite few parameters too. Right. I mentioned that I missed the lack of function comments when looking at #3 and asked Alan to go back and fix them in #1 and #2. Second, this patch should also fix PR 67283. It would be great if you could verify that and add it to the changelog when committing if that is indeed the case. Excellent. Yes, definitely mention the BZ. jeff
Re: [PATCH 2/5] completely_scalarize arrays as well as records
Hi,
On Tue, Aug 25, 2015 at 12:06:14PM +0100, Alan Lawrence wrote:
> This changes the completely_scalarize_record path to also work on arrays (thus
> allowing records containing arrays, etc.). This just required extending the
> existing type_consists_of_records_p and completely_scalarize_record methods
> to handle things of ARRAY_TYPE as well as RECORD_TYPE. Hence, I renamed both
> methods so as not to mention 'record'.
thanks for working on this. I see Jeff has already approved the
patch, but I have two comments nevertheless. First, I would be much
happier if you added a proper comment to scalarize_elem function which
you forgot completely. The name is not very descriptive and it has
quite few parameters too.
Second, this patch should also fix PR 67283. It would be great if you
could verify that and add it to the changelog when committing if that
is indeed the case.
Thanks,
Martin
>
> Bootstrapped + check-gcc on aarch64-none-linux-gnu, arm-none-linux-gnueabihf
> and x86_64-none-linux-gnu.
>
> Have also verified the scan-tree-dump check in the new sra-15.c passes (using
> a stage 1 compiler only, no execution test) on alpha, hppa, powerpc, sparc,
> avr and sh.
>
> gcc/ChangeLog:
>
> * tree-sra.c (type_consists_of_records_p): Rename to...
> (scalarizable_type_p): ...this, add case for ARRAY_TYPE.
>
> (completely_scalarize_record): Rename to...
> (completely_scalarize): ...this, add ARRAY_TYPE case, move some code to:
> (scalarize_elem): New.
>
> gcc/testsuite/ChangeLog:
> * gcc.dg/tree-ssa/sra-15.c: New.
> ---
> gcc/testsuite/gcc.dg/tree-ssa/sra-15.c | 38 +
> gcc/tree-sra.c | 146
> ++---
> 2 files changed, 135 insertions(+), 49 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
> b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
> new file mode 100644
> index 000..e251058
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
> @@ -0,0 +1,38 @@
> +/* Verify that SRA total scalarization works on records containing arrays.
> */
> +/* Test skipped for targets with small (often default) MOVE_RATIO. */
> +/* { dg-do run } */
> +/* { dg-options "-O1 -fdump-tree-release_ssa --param
> sra-max-scalarization-size-Ospeed=32" } */
> +
> +extern void abort (void);
> +
> +struct S
> +{
> + char c;
> + unsigned short f[2][2];
> + int i;
> + unsigned short f3, f4;
> +};
> +
> +
> +int __attribute__ ((noinline))
> +foo (struct S *p)
> +{
> + struct S l;
> +
> + l = *p;
> + l.i++;
> + l.f[1][0] += 3;
> + *p = l;
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> + struct S a = {0, { {5, 7}, {9, 11} }, 4, 0, 0};
> + foo (&a);
> + if (a.i != 5 || a.f[1][0] != 12)
> +abort ();
> + return 0;
> +}
> +
> +/* { dg-final { scan-tree-dump-times "l;" 0 "release_ssa" } } */
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index a0c92b0..08fa8dc 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -915,74 +915,122 @@ create_access (tree expr, gimple stmt, bool write)
> }
>
>
> -/* Return true iff TYPE is a RECORD_TYPE with fields that are either of
> gimple
> - register types or (recursively) records with only these two kinds of
> fields.
> - It also returns false if any of these records contains a bit-field. */
> +/* Return true iff TYPE is scalarizable - i.e. a RECORD_TYPE or ARRAY_TYPE
> with
> + fields that are either of gimple register types (excluding bit-fields)
> + or (recursively) scalarizable types. */
>
> static bool
> -type_consists_of_records_p (tree type)
> +scalarizable_type_p (tree type)
> {
> - tree fld;
> + gcc_assert (!is_gimple_reg_type (type));
>
> - if (TREE_CODE (type) != RECORD_TYPE)
> -return false;
> + switch (TREE_CODE (type))
> + {
> + case RECORD_TYPE:
> +for (tree fld = TYPE_FIELDS (type); fld; fld = DECL_CHAIN (fld))
> + if (TREE_CODE (fld) == FIELD_DECL)
> + {
> + tree ft = TREE_TYPE (fld);
>
> - for (fld = TYPE_FIELDS (type); fld; fld = DECL_CHAIN (fld))
> -if (TREE_CODE (fld) == FIELD_DECL)
> - {
> - tree ft = TREE_TYPE (fld);
> + if (DECL_BIT_FIELD (fld))
> + return false;
>
> - if (DECL_BIT_FIELD (fld))
> - return false;
> + if (!is_gimple_reg_type (ft)
> + && !scalarizable_type_p (ft))
> + return false;
> + }
>
> - if (!is_gimple_reg_type (ft)
> - && !type_consists_of_records_p (ft))
> - return false;
> - }
> +return true;
>
> - return true;
> + case ARRAY_TYPE:
> +{
> + tree elem = TREE_TYPE (type);
> + if (DECL_P (elem) && DECL_BIT_FIELD (elem))
> + return false;
> + if (!is_gimple_reg_type (elem)
> + && !scalarizable_type_p (elem))
> + return false;
> + return true;
> +}
> + default:
> +return false;
> + }
> }
>
> -/* Create total_scalarization accesses for all sca
Re: [PATCH 2/5] completely_scalarize arrays as well as records
On 08/25/2015 05:06 AM, Alan Lawrence wrote: This changes the completely_scalarize_record path to also work on arrays (thus allowing records containing arrays, etc.). This just required extending the existing type_consists_of_records_p and completely_scalarize_record methods to handle things of ARRAY_TYPE as well as RECORD_TYPE. Hence, I renamed both methods so as not to mention 'record'. Bootstrapped + check-gcc on aarch64-none-linux-gnu, arm-none-linux-gnueabihf and x86_64-none-linux-gnu. Have also verified the scan-tree-dump check in the new sra-15.c passes (using a stage 1 compiler only, no execution test) on alpha, hppa, powerpc, sparc, avr and sh. gcc/ChangeLog: * tree-sra.c (type_consists_of_records_p): Rename to... (scalarizable_type_p): ...this, add case for ARRAY_TYPE. (completely_scalarize_record): Rename to... (completely_scalarize): ...this, add ARRAY_TYPE case, move some code to: (scalarize_elem): New. gcc/testsuite/ChangeLog: * gcc.dg/tree-ssa/sra-15.c: New. --- gcc/testsuite/gcc.dg/tree-ssa/sra-15.c | 38 + gcc/tree-sra.c | 146 ++--- 2 files changed, 135 insertions(+), 49 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/sra-15.c diff --git a/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c new file mode 100644 index 000..e251058 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c @@ -0,0 +1,38 @@ +/* Verify that SRA total scalarization works on records containing arrays. */ +/* Test skipped for targets with small (often default) MOVE_RATIO. */ ?!? I don't see anything that skips this test for any targets. Presumably this was copied from sra-12.c. I suspect this comment should just be removed. With that comment removed from the testcase, this is OK. jeff
[PATCH 2/5] completely_scalarize arrays as well as records
This changes the completely_scalarize_record path to also work on arrays (thus
allowing records containing arrays, etc.). This just required extending the
existing type_consists_of_records_p and completely_scalarize_record methods
to handle things of ARRAY_TYPE as well as RECORD_TYPE. Hence, I renamed both
methods so as not to mention 'record'.
Bootstrapped + check-gcc on aarch64-none-linux-gnu, arm-none-linux-gnueabihf
and x86_64-none-linux-gnu.
Have also verified the scan-tree-dump check in the new sra-15.c passes (using a
stage 1 compiler only, no execution test) on alpha, hppa, powerpc, sparc, avr
and sh.
gcc/ChangeLog:
* tree-sra.c (type_consists_of_records_p): Rename to...
(scalarizable_type_p): ...this, add case for ARRAY_TYPE.
(completely_scalarize_record): Rename to...
(completely_scalarize): ...this, add ARRAY_TYPE case, move some code to:
(scalarize_elem): New.
gcc/testsuite/ChangeLog:
* gcc.dg/tree-ssa/sra-15.c: New.
---
gcc/testsuite/gcc.dg/tree-ssa/sra-15.c | 38 +
gcc/tree-sra.c | 146 ++---
2 files changed, 135 insertions(+), 49 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
new file mode 100644
index 000..e251058
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
@@ -0,0 +1,38 @@
+/* Verify that SRA total scalarization works on records containing arrays. */
+/* Test skipped for targets with small (often default) MOVE_RATIO. */
+/* { dg-do run } */
+/* { dg-options "-O1 -fdump-tree-release_ssa --param
sra-max-scalarization-size-Ospeed=32" } */
+
+extern void abort (void);
+
+struct S
+{
+ char c;
+ unsigned short f[2][2];
+ int i;
+ unsigned short f3, f4;
+};
+
+
+int __attribute__ ((noinline))
+foo (struct S *p)
+{
+ struct S l;
+
+ l = *p;
+ l.i++;
+ l.f[1][0] += 3;
+ *p = l;
+}
+
+int
+main (int argc, char **argv)
+{
+ struct S a = {0, { {5, 7}, {9, 11} }, 4, 0, 0};
+ foo (&a);
+ if (a.i != 5 || a.f[1][0] != 12)
+abort ();
+ return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "l;" 0 "release_ssa" } } */
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index a0c92b0..08fa8dc 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -915,74 +915,122 @@ create_access (tree expr, gimple stmt, bool write)
}
-/* Return true iff TYPE is a RECORD_TYPE with fields that are either of gimple
- register types or (recursively) records with only these two kinds of fields.
- It also returns false if any of these records contains a bit-field. */
+/* Return true iff TYPE is scalarizable - i.e. a RECORD_TYPE or ARRAY_TYPE with
+ fields that are either of gimple register types (excluding bit-fields)
+ or (recursively) scalarizable types. */
static bool
-type_consists_of_records_p (tree type)
+scalarizable_type_p (tree type)
{
- tree fld;
+ gcc_assert (!is_gimple_reg_type (type));
- if (TREE_CODE (type) != RECORD_TYPE)
-return false;
+ switch (TREE_CODE (type))
+ {
+ case RECORD_TYPE:
+for (tree fld = TYPE_FIELDS (type); fld; fld = DECL_CHAIN (fld))
+ if (TREE_CODE (fld) == FIELD_DECL)
+ {
+ tree ft = TREE_TYPE (fld);
- for (fld = TYPE_FIELDS (type); fld; fld = DECL_CHAIN (fld))
-if (TREE_CODE (fld) == FIELD_DECL)
- {
- tree ft = TREE_TYPE (fld);
+ if (DECL_BIT_FIELD (fld))
+ return false;
- if (DECL_BIT_FIELD (fld))
- return false;
+ if (!is_gimple_reg_type (ft)
+ && !scalarizable_type_p (ft))
+ return false;
+ }
- if (!is_gimple_reg_type (ft)
- && !type_consists_of_records_p (ft))
- return false;
- }
+return true;
- return true;
+ case ARRAY_TYPE:
+{
+ tree elem = TREE_TYPE (type);
+ if (DECL_P (elem) && DECL_BIT_FIELD (elem))
+ return false;
+ if (!is_gimple_reg_type (elem)
+&& !scalarizable_type_p (elem))
+ return false;
+ return true;
+}
+ default:
+return false;
+ }
}
-/* Create total_scalarization accesses for all scalar type fields in DECL that
- must be of a RECORD_TYPE conforming to type_consists_of_records_p. BASE
- must be the top-most VAR_DECL representing the variable, OFFSET must be the
- offset of DECL within BASE. REF must be the memory reference expression for
- the given decl. */
+static void scalarize_elem (tree, HOST_WIDE_INT, HOST_WIDE_INT, tree, tree);
+
+/* Create total_scalarization accesses for all scalar fields of a member
+ of type DECL_TYPE conforming to scalarizable_type_p. BASE
+ must be the top-most VAR_DECL representing the variable; within that,
+ OFFSET locates the member and REF must be the memory reference expression
for
+ the member. */
static void
-completely_scalarize_record (tree base, tree decl, HOST_WIDE_INT offset,
-tree
