* [PATCH] vect: Tighten check for SLP memory groups [PR103517]
@ 2021-12-01 10:45 Richard Sandiford
2021-12-01 13:14 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Richard Sandiford @ 2021-12-01 10:45 UTC (permalink / raw)
To: gcc-patches
When checking for compatible stmts, vect_build_slp_tree_1 did:
&& !(STMT_VINFO_GROUPED_ACCESS (stmt_info)
&& (first_stmt_code == ARRAY_REF
|| first_stmt_code == BIT_FIELD_REF
|| first_stmt_code == INDIRECT_REF
|| first_stmt_code == COMPONENT_REF
|| first_stmt_code == MEM_REF)))
That is, it allowed any rhs_code as long as the first_stmt_code
looked valid. This had the effect of allowing IFN_MASK_LOAD
to be paired with an earlier non-call code (but didn't allow
the reverse).
This patch makes the check symmetrical.
Still testing on x86_64-linux-gnu. OK if testing passes, or doesn't
this seem like the right approach?
Richard
gcc/
PR tree-optimization/103517
* tree-vect-slp.c (vect_build_slp_tree_1): When allowing two
different component references, check the codes of both them,
rather than just the first.
gcc/testsuite/
PR tree-optimization/103517
* gcc.dg/vect/pr103517.c: New test.
---
gcc/testsuite/gcc.dg/vect/pr103517.c | 13 +++++++++++++
gcc/tree-vect-slp.c | 7 ++++++-
2 files changed, 19 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.dg/vect/pr103517.c
diff --git a/gcc/testsuite/gcc.dg/vect/pr103517.c b/gcc/testsuite/gcc.dg/vect/pr103517.c
new file mode 100644
index 00000000000..de87fc48f84
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vect/pr103517.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-march=skylake-avx512" { target x86_64-*-* i?86-*-* } } */
+
+int a;
+short b, c;
+extern short d[];
+void e() {
+ for (short f = 1; f < (short)a; f += 2)
+ if (d[f + 1]) {
+ b = d[f];
+ c = d[f + 1];
+ }
+}
diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
index 7bff5118bd0..bc22ffeed82 100644
--- a/gcc/tree-vect-slp.c
+++ b/gcc/tree-vect-slp.c
@@ -1121,7 +1121,12 @@ vect_build_slp_tree_1 (vec_info *vinfo, unsigned char *swap,
|| first_stmt_code == BIT_FIELD_REF
|| first_stmt_code == INDIRECT_REF
|| first_stmt_code == COMPONENT_REF
- || first_stmt_code == MEM_REF)))
+ || first_stmt_code == MEM_REF)
+ && (rhs_code == ARRAY_REF
+ || rhs_code == BIT_FIELD_REF
+ || rhs_code == INDIRECT_REF
+ || rhs_code == COMPONENT_REF
+ || rhs_code == MEM_REF)))
|| first_stmt_load_p != load_p
|| first_stmt_phi_p != phi_p)
{
--
2.25.1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] vect: Tighten check for SLP memory groups [PR103517]
2021-12-01 10:45 [PATCH] vect: Tighten check for SLP memory groups [PR103517] Richard Sandiford
@ 2021-12-01 13:14 ` Richard Biener
2021-12-01 13:25 ` Richard Sandiford
0 siblings, 1 reply; 4+ messages in thread
From: Richard Biener @ 2021-12-01 13:14 UTC (permalink / raw)
To: Richard Sandiford, GCC Patches
On Wed, Dec 1, 2021 at 11:56 AM Richard Sandiford via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> When checking for compatible stmts, vect_build_slp_tree_1 did:
>
> && !(STMT_VINFO_GROUPED_ACCESS (stmt_info)
> && (first_stmt_code == ARRAY_REF
> || first_stmt_code == BIT_FIELD_REF
> || first_stmt_code == INDIRECT_REF
> || first_stmt_code == COMPONENT_REF
> || first_stmt_code == MEM_REF)))
>
> That is, it allowed any rhs_code as long as the first_stmt_code
> looked valid. This had the effect of allowing IFN_MASK_LOAD
> to be paired with an earlier non-call code (but didn't allow
> the reverse).
>
> This patch makes the check symmetrical.
>
> Still testing on x86_64-linux-gnu. OK if testing passes, or doesn't
> this seem like the right approach?
It's indeed a too weak comparison of the classification of the first
and the followup operands, some larger refactoring is probably
needed to improve here (note how we compare STMT_VINFO_GROUPED_ACCESS
of the followup against the tree codes of the first stmt but also later
compare first_stmt_load_p against load_p).
The proposed patch looks reasonable (but then we could drop
the STMT_VINFO_GROUPED_ACCESS (stmt_info) part of the check?),
so OK.
Thanks,
Richard.
> Richard
>
>
> gcc/
> PR tree-optimization/103517
> * tree-vect-slp.c (vect_build_slp_tree_1): When allowing two
> different component references, check the codes of both them,
> rather than just the first.
>
> gcc/testsuite/
> PR tree-optimization/103517
> * gcc.dg/vect/pr103517.c: New test.
> ---
> gcc/testsuite/gcc.dg/vect/pr103517.c | 13 +++++++++++++
> gcc/tree-vect-slp.c | 7 ++++++-
> 2 files changed, 19 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.dg/vect/pr103517.c
>
> diff --git a/gcc/testsuite/gcc.dg/vect/pr103517.c b/gcc/testsuite/gcc.dg/vect/pr103517.c
> new file mode 100644
> index 00000000000..de87fc48f84
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vect/pr103517.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-march=skylake-avx512" { target x86_64-*-* i?86-*-* } } */
> +
> +int a;
> +short b, c;
> +extern short d[];
> +void e() {
> + for (short f = 1; f < (short)a; f += 2)
> + if (d[f + 1]) {
> + b = d[f];
> + c = d[f + 1];
> + }
> +}
> diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
> index 7bff5118bd0..bc22ffeed82 100644
> --- a/gcc/tree-vect-slp.c
> +++ b/gcc/tree-vect-slp.c
> @@ -1121,7 +1121,12 @@ vect_build_slp_tree_1 (vec_info *vinfo, unsigned char *swap,
> || first_stmt_code == BIT_FIELD_REF
> || first_stmt_code == INDIRECT_REF
> || first_stmt_code == COMPONENT_REF
> - || first_stmt_code == MEM_REF)))
> + || first_stmt_code == MEM_REF)
> + && (rhs_code == ARRAY_REF
> + || rhs_code == BIT_FIELD_REF
> + || rhs_code == INDIRECT_REF
> + || rhs_code == COMPONENT_REF
> + || rhs_code == MEM_REF)))
> || first_stmt_load_p != load_p
> || first_stmt_phi_p != phi_p)
> {
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] vect: Tighten check for SLP memory groups [PR103517]
2021-12-01 13:14 ` Richard Biener
@ 2021-12-01 13:25 ` Richard Sandiford
2021-12-01 13:28 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Richard Sandiford @ 2021-12-01 13:25 UTC (permalink / raw)
To: Richard Biener via Gcc-patches
Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On Wed, Dec 1, 2021 at 11:56 AM Richard Sandiford via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> When checking for compatible stmts, vect_build_slp_tree_1 did:
>>
>> && !(STMT_VINFO_GROUPED_ACCESS (stmt_info)
>> && (first_stmt_code == ARRAY_REF
>> || first_stmt_code == BIT_FIELD_REF
>> || first_stmt_code == INDIRECT_REF
>> || first_stmt_code == COMPONENT_REF
>> || first_stmt_code == MEM_REF)))
>>
>> That is, it allowed any rhs_code as long as the first_stmt_code
>> looked valid. This had the effect of allowing IFN_MASK_LOAD
>> to be paired with an earlier non-call code (but didn't allow
>> the reverse).
>>
>> This patch makes the check symmetrical.
>>
>> Still testing on x86_64-linux-gnu. OK if testing passes, or doesn't
>> this seem like the right approach?
>
> It's indeed a too weak comparison of the classification of the first
> and the followup operands, some larger refactoring is probably
> needed to improve here (note how we compare STMT_VINFO_GROUPED_ACCESS
> of the followup against the tree codes of the first stmt but also later
> compare first_stmt_load_p against load_p).
>
> The proposed patch looks reasonable (but then we could drop
> the STMT_VINFO_GROUPED_ACCESS (stmt_info) part of the check?),
Yeah, was wondering about that. Seemed safer to keep it, since without
it we might pair non-memory BIT_FIELD_REFs with other things. I guess
the same goes for the first stmt though, and the mismatch ought to be
caught later anyway.
> so OK.
Thanks,
Richard
> Thanks,
> Richard.
>
>> Richard
>>
>>
>> gcc/
>> PR tree-optimization/103517
>> * tree-vect-slp.c (vect_build_slp_tree_1): When allowing two
>> different component references, check the codes of both them,
>> rather than just the first.
>>
>> gcc/testsuite/
>> PR tree-optimization/103517
>> * gcc.dg/vect/pr103517.c: New test.
>> ---
>> gcc/testsuite/gcc.dg/vect/pr103517.c | 13 +++++++++++++
>> gcc/tree-vect-slp.c | 7 ++++++-
>> 2 files changed, 19 insertions(+), 1 deletion(-)
>> create mode 100644 gcc/testsuite/gcc.dg/vect/pr103517.c
>>
>> diff --git a/gcc/testsuite/gcc.dg/vect/pr103517.c b/gcc/testsuite/gcc.dg/vect/pr103517.c
>> new file mode 100644
>> index 00000000000..de87fc48f84
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/vect/pr103517.c
>> @@ -0,0 +1,13 @@
>> +/* { dg-do compile } */
>> +/* { dg-additional-options "-march=skylake-avx512" { target x86_64-*-* i?86-*-* } } */
>> +
>> +int a;
>> +short b, c;
>> +extern short d[];
>> +void e() {
>> + for (short f = 1; f < (short)a; f += 2)
>> + if (d[f + 1]) {
>> + b = d[f];
>> + c = d[f + 1];
>> + }
>> +}
>> diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
>> index 7bff5118bd0..bc22ffeed82 100644
>> --- a/gcc/tree-vect-slp.c
>> +++ b/gcc/tree-vect-slp.c
>> @@ -1121,7 +1121,12 @@ vect_build_slp_tree_1 (vec_info *vinfo, unsigned char *swap,
>> || first_stmt_code == BIT_FIELD_REF
>> || first_stmt_code == INDIRECT_REF
>> || first_stmt_code == COMPONENT_REF
>> - || first_stmt_code == MEM_REF)))
>> + || first_stmt_code == MEM_REF)
>> + && (rhs_code == ARRAY_REF
>> + || rhs_code == BIT_FIELD_REF
>> + || rhs_code == INDIRECT_REF
>> + || rhs_code == COMPONENT_REF
>> + || rhs_code == MEM_REF)))
>> || first_stmt_load_p != load_p
>> || first_stmt_phi_p != phi_p)
>> {
>> --
>> 2.25.1
>>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] vect: Tighten check for SLP memory groups [PR103517]
2021-12-01 13:25 ` Richard Sandiford
@ 2021-12-01 13:28 ` Richard Biener
0 siblings, 0 replies; 4+ messages in thread
From: Richard Biener @ 2021-12-01 13:28 UTC (permalink / raw)
To: Richard Biener via Gcc-patches, Richard Biener, Richard Sandiford
On Wed, Dec 1, 2021 at 2:25 PM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > On Wed, Dec 1, 2021 at 11:56 AM Richard Sandiford via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> When checking for compatible stmts, vect_build_slp_tree_1 did:
> >>
> >> && !(STMT_VINFO_GROUPED_ACCESS (stmt_info)
> >> && (first_stmt_code == ARRAY_REF
> >> || first_stmt_code == BIT_FIELD_REF
> >> || first_stmt_code == INDIRECT_REF
> >> || first_stmt_code == COMPONENT_REF
> >> || first_stmt_code == MEM_REF)))
> >>
> >> That is, it allowed any rhs_code as long as the first_stmt_code
> >> looked valid. This had the effect of allowing IFN_MASK_LOAD
> >> to be paired with an earlier non-call code (but didn't allow
> >> the reverse).
> >>
> >> This patch makes the check symmetrical.
> >>
> >> Still testing on x86_64-linux-gnu. OK if testing passes, or doesn't
> >> this seem like the right approach?
> >
> > It's indeed a too weak comparison of the classification of the first
> > and the followup operands, some larger refactoring is probably
> > needed to improve here (note how we compare STMT_VINFO_GROUPED_ACCESS
> > of the followup against the tree codes of the first stmt but also later
> > compare first_stmt_load_p against load_p).
> >
> > The proposed patch looks reasonable (but then we could drop
> > the STMT_VINFO_GROUPED_ACCESS (stmt_info) part of the check?),
>
> Yeah, was wondering about that. Seemed safer to keep it, since without
> it we might pair non-memory BIT_FIELD_REFs with other things. I guess
> the same goes for the first stmt though, and the mismatch ought to be
> caught later anyway.
Hmm, yeah. I guess some classify_stmt () returning a custom enum
and comparing that might clean up this whole thing. But nothing for stage3.
Richard.
> > so OK.
>
> Thanks,
> Richard
>
> > Thanks,
> > Richard.
> >
> >> Richard
> >>
> >>
> >> gcc/
> >> PR tree-optimization/103517
> >> * tree-vect-slp.c (vect_build_slp_tree_1): When allowing two
> >> different component references, check the codes of both them,
> >> rather than just the first.
> >>
> >> gcc/testsuite/
> >> PR tree-optimization/103517
> >> * gcc.dg/vect/pr103517.c: New test.
> >> ---
> >> gcc/testsuite/gcc.dg/vect/pr103517.c | 13 +++++++++++++
> >> gcc/tree-vect-slp.c | 7 ++++++-
> >> 2 files changed, 19 insertions(+), 1 deletion(-)
> >> create mode 100644 gcc/testsuite/gcc.dg/vect/pr103517.c
> >>
> >> diff --git a/gcc/testsuite/gcc.dg/vect/pr103517.c b/gcc/testsuite/gcc.dg/vect/pr103517.c
> >> new file mode 100644
> >> index 00000000000..de87fc48f84
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/vect/pr103517.c
> >> @@ -0,0 +1,13 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-additional-options "-march=skylake-avx512" { target x86_64-*-* i?86-*-* } } */
> >> +
> >> +int a;
> >> +short b, c;
> >> +extern short d[];
> >> +void e() {
> >> + for (short f = 1; f < (short)a; f += 2)
> >> + if (d[f + 1]) {
> >> + b = d[f];
> >> + c = d[f + 1];
> >> + }
> >> +}
> >> diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
> >> index 7bff5118bd0..bc22ffeed82 100644
> >> --- a/gcc/tree-vect-slp.c
> >> +++ b/gcc/tree-vect-slp.c
> >> @@ -1121,7 +1121,12 @@ vect_build_slp_tree_1 (vec_info *vinfo, unsigned char *swap,
> >> || first_stmt_code == BIT_FIELD_REF
> >> || first_stmt_code == INDIRECT_REF
> >> || first_stmt_code == COMPONENT_REF
> >> - || first_stmt_code == MEM_REF)))
> >> + || first_stmt_code == MEM_REF)
> >> + && (rhs_code == ARRAY_REF
> >> + || rhs_code == BIT_FIELD_REF
> >> + || rhs_code == INDIRECT_REF
> >> + || rhs_code == COMPONENT_REF
> >> + || rhs_code == MEM_REF)))
> >> || first_stmt_load_p != load_p
> >> || first_stmt_phi_p != phi_p)
> >> {
> >> --
> >> 2.25.1
> >>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-12-01 13:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-01 10:45 [PATCH] vect: Tighten check for SLP memory groups [PR103517] Richard Sandiford
2021-12-01 13:14 ` Richard Biener
2021-12-01 13:25 ` Richard Sandiford
2021-12-01 13:28 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).