| Message ID | d6db3e30-7193-0fb5-2e64-02dd34f8da69@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | vect, omp: inbranch simdclone dropping const | expand |
I don't have authority to approve anything, but here's a review anyway. Thanks for working on this. On 26/09/2023 17:24, Andre Vieira (lists) wrote: > The const attribute is ignored when simdclone's are used inbranch. This > is due to the fact that when analyzing a MASK_CALL we were not looking > at the targeted function for flags, but instead only at the internal > function call itself. > This patch adds code to make sure we look at the target function to > check for the const attribute and enables the autovectorization of > inbranch const simdclones without needing the loop to be adorned the > 'openmp simd' pragma. > > Not sure about how to add new includes to the ChangeLog. Which brings me > to another point, I contemplated changing gimple_call_flags to do the > checking of flags of the first argument of IFN_MASK_CALL itself rather > than only calling internal_fn_flags on gimple_call_internal_fn (stmt), > but that might be a bit too intrusive, opinions welcome :) > > Bootstrapped and regression tested on aarch64-unknown-linux-gnu and > x86_64-pc-linux-gnu. > > Is this OK for trunk? > > gcc/ChangeLog: > > * tree-vect-data-ref.cc (include calls.h): Add new include. > (get_references_in_stmt): Correctly handle IFN_MASK_CALL. > > gcc/testsuite/ChangeLog: > > * gcc.dg/vect/vect-simd-clone-19.c: New test. > diff --git a/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c > new file mode 100644 > index 0000000000000000000000000000000000000000..09127b8cb6f2e3699b6073591f58be7047330273 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c > @@ -0,0 +1,23 @@ > +/* { dg-require-effective-target vect_simd_clones } */ > +/* { dg-do compile } */ > +/* { dg-additional-options "-fopenmp-simd" } */ > + Do you need -fopenmp-simd for this? > + tree orig_fndecl > + = gimple_call_addr_fndecl (gimple_call_arg (stmt, 0)); > + if (!orig_fndecl) > + { > + clobbers_memory = true; > + break; > + } > + if ((flags_from_decl_or_type (orig_fndecl) & ECF_CONST) == 0) > + clobbers_memory = true; > + } > break; > Can be simplified: if (!orig_fndecl || (flags_from_decl_or_type (orig_fndecl) & ECF_CONST) == 0) clobbers_memory = true; break; > @@ -5852,7 +5865,7 @@ get_references_in_stmt (gimple *stmt, vec<data_ref_loc, va_heap> *references) > } > else if (stmt_code == GIMPLE_CALL) > { > - unsigned i, n; > + unsigned i = 0, n; > tree ptr, type; > unsigned int align; > Rogue space. > @@ -5879,13 +5892,15 @@ get_references_in_stmt (gimple *stmt, vec<data_ref_loc, va_heap> *references) > ptr); > references->safe_push (ref); > return false; > + case IFN_MASK_CALL: > + i = 1; > default: > break; > } > If the fall-through is deliberate please add a /* FALLTHROUGH */ comment (or whatever spelling disables the warning). Andrew
On 26.09.23 18:37, Andrew Stubbs wrote: > If the fall-through is deliberate please add a /* FALLTHROUGH */ > comment (or whatever spelling disables the warning). It's: gcc_fallthrough (); Which gets converted to "__attribute__((fallthrough))"; it could also expand to "[[fallthrough]]" but that's C++17 (and, also, an C23 feature - albeit so far unimplemented in gcc). Tobias ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
On Tue, Sep 26, 2023 at 05:24:26PM +0100, Andre Vieira (lists) wrote: > @@ -5816,6 +5817,18 @@ get_references_in_stmt (gimple *stmt, vec<data_ref_loc, va_heap> *references) > } > case IFN_MASK_LOAD: > case IFN_MASK_STORE: > + case IFN_MASK_CALL: > + { > + tree orig_fndecl > + = gimple_call_addr_fndecl (gimple_call_arg (stmt, 0)); > + if (!orig_fndecl) > + { > + clobbers_memory = true; > + break; > + } > + if ((flags_from_decl_or_type (orig_fndecl) & ECF_CONST) == 0) > + clobbers_memory = true; > + } Should IFN_MASK_LOAD/STORE really go through this? I thought those have first argument address of the memory being conditionally loaded/stored, not function address. Jakub
On 26 September 2023 18:46:11 CEST, Tobias Burnus <tobias@codesourcery.com> wrote: >On 26.09.23 18:37, Andrew Stubbs wrote: >> If the fall-through is deliberate please add a /* FALLTHROUGH */ >> comment (or whatever spelling disables the warning). > >It's: gcc_fallthrough (); > >Which gets converted to "__attribute__((fallthrough))"; it could also >expand to "[[fallthrough]]" but that's C++17 (and, also, an C23 feature >- albeit so far unimplemented in gcc). OT IIRC we do parse comments for a number of spellings of the hint by the user that the fallthrough is deliberate: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html See the numerous levels of -Wimplicit-fallthrough=n, the default being 3. ---8<--- -Wimplicit-fallthrough=3 case sensitively matches one of the following regular expressions: -fallthrough @fallthrough@ lint -fallthrough[ \t]* [ \t.!]*(ELSE,? |INTENTIONAL(LY)? )? FALL(S | |-)?THR(OUGH|U)[ \t.!]*(-[^\n\r]*)? [ \t.!]*(Else,? |Intentional(ly)? )? Fall((s | |-)[Tt]|t)hr(ough|u)[ \t.!]*(-[^\n\r]*)? [ \t.!]*([Ee]lse,? |[Ii]ntentional(ly)? )? fall(s | |-)?thr(ough|u)[ \t.!]*(-[^\n\r]*)? ---8<--- Just FWIW. thanks,
On 26/09/2023 17:48, Jakub Jelinek wrote: > On Tue, Sep 26, 2023 at 05:24:26PM +0100, Andre Vieira (lists) wrote: >> @@ -5816,6 +5817,18 @@ get_references_in_stmt (gimple *stmt, vec<data_ref_loc, va_heap> *references) >> } >> case IFN_MASK_LOAD: >> case IFN_MASK_STORE: >> + case IFN_MASK_CALL: >> + { >> + tree orig_fndecl >> + = gimple_call_addr_fndecl (gimple_call_arg (stmt, 0)); >> + if (!orig_fndecl) >> + { >> + clobbers_memory = true; >> + break; >> + } >> + if ((flags_from_decl_or_type (orig_fndecl) & ECF_CONST) == 0) >> + clobbers_memory = true; >> + } > > Should IFN_MASK_LOAD/STORE really go through this? I thought those have > first argument address of the memory being conditionally loaded/stored, not > function address. > No it shouldn't, my bad... Surprising testing didn't catch it though, I'm guessing gimple_call_addr_fndecl just returned null everytime for those. I'll clean it up.
On 26/09/2023 21:26, Bernhard Reutner-Fischer wrote: > On 26 September 2023 18:46:11 CEST, Tobias Burnus <tobias@codesourcery.com> wrote: >> On 26.09.23 18:37, Andrew Stubbs wrote: >>> If the fall-through is deliberate please add a /* FALLTHROUGH */ >>> comment (or whatever spelling disables the warning). >> >> It's: gcc_fallthrough (); >> >> Which gets converted to "__attribute__((fallthrough))"; it could also >> expand to "[[fallthrough]]" but that's C++17 (and, also, an C23 feature >> - albeit so far unimplemented in gcc). > > OT > IIRC we do parse comments for a number of spellings of the hint by the user that the fallthrough is deliberate: > > https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html > > See the numerous levels of -Wimplicit-fallthrough=n, the default being 3. > > ---8<--- > -Wimplicit-fallthrough=3 case sensitively matches one of the following regular expressions: > -fallthrough > @fallthrough@ > lint -fallthrough[ \t]* > [ \t.!]*(ELSE,? |INTENTIONAL(LY)? )? > FALL(S | |-)?THR(OUGH|U)[ \t.!]*(-[^\n\r]*)? > [ \t.!]*(Else,? |Intentional(ly)? )? > Fall((s | |-)[Tt]|t)hr(ough|u)[ \t.!]*(-[^\n\r]*)? > [ \t.!]*([Ee]lse,? |[Ii]ntentional(ly)? )? > fall(s | |-)?thr(ough|u)[ \t.!]*(-[^\n\r]*)? > ---8<--- > > Just FWIW. > thanks, I was surprised my bootstrap didn't catch this, I thought we generated warnings in such cases and bootstrap builds with -Werror does it not?
On 26 September 2023 23:02:10 CEST, "Andre Vieira (lists)" <andre.simoesdiasvieira@arm.com> wrote: > > >On 26/09/2023 21:26, Bernhard Reutner-Fischer wrote: >> On 26 September 2023 18:46:11 CEST, Tobias Burnus <tobias@codesourcery.com> wrote: >>> On 26.09.23 18:37, Andrew Stubbs wrote: >>>> If the fall-through is deliberate please add a /* FALLTHROUGH */ >>>> comment (or whatever spelling disables the warning). >>> > >I was surprised my bootstrap didn't catch this, I thought we generated warnings in such cases and bootstrap builds with -Werror does it not? Well, I wouldn't see much benefit to warn in this case, no? You're falling through to a break, not other "active" code AFAICS? You had: references->safe_push (ref); return false; + case IFN_MASK_CALL: + i = 1; default: break; } I would not have expected a warning here, TBH :-) thanks,
On 26/09/2023 17:37, Andrew Stubbs wrote: > I don't have authority to approve anything, but here's a review anyway. > > Thanks for working on this. Thank you for reviewing and apologies for the mess of a patch, may have rushed it ;) > >> diff --git a/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c >> b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c >> new file mode 100644 >> index >> 0000000000000000000000000000000000000000..09127b8cb6f2e3699b6073591f58be7047330273 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c >> @@ -0,0 +1,23 @@ >> +/* { dg-require-effective-target vect_simd_clones } */ >> +/* { dg-do compile } */ >> +/* { dg-additional-options "-fopenmp-simd" } */ >> + > > Do you need -fopenmp-simd for this? Nope, I keep forgetting that you only need it for pragmas. Dealt with the other comments too. Any thoughts on changing gimple_call_internal_fn instead? My main argument against is that IFN_MASK_CALL should not appear outside of ifconvert and vectorizer. On the other hand, we may inspect the flags elsewhere in the vectorizer (now or in the future) and changing gimple_call_internal_fn would prevent the need to handle the IFN separately elsewhere. Kind Regards, Andre diff --git a/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c new file mode 100644 index 0000000000000000000000000000000000000000..e7ed56ca75470464307d0d266dacfa0d8d6e43c1 --- /dev/null +++ b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c @@ -0,0 +1,22 @@ +/* { dg-require-effective-target vect_simd_clones } */ +/* { dg-do compile } */ + +int __attribute__ ((__simd__, const)) fn (int); + +void test (int * __restrict__ a, int * __restrict__ b, int n) +{ + for (int i = 0; i < n; ++i) + { + int a_; + if (b[i] > 0) + a_ = fn (b[i]); + else + a_ = b[i] + 5; + a[i] = a_; + } +} + +/* { dg-final { scan-tree-dump-not {loop contains function calls or data references} "vect" } } */ + +/* The LTO test produces two dump files and we scan the wrong one. */ +/* { dg-skip-if "" { *-*-* } { "-flto" } { "" } } */ diff --git a/gcc/tree-data-ref.cc b/gcc/tree-data-ref.cc index 6d3b7c2290e4db9c1168a4c763facb481157c97c..689aaeed72282bb0da2a17e19fb923a06e8d62fa 100644 --- a/gcc/tree-data-ref.cc +++ b/gcc/tree-data-ref.cc @@ -100,6 +100,7 @@ along with GCC; see the file COPYING3. If not see #include "vr-values.h" #include "range-op.h" #include "tree-ssa-loop-ivopts.h" +#include "calls.h" static struct datadep_stats { @@ -5816,6 +5817,15 @@ get_references_in_stmt (gimple *stmt, vec<data_ref_loc, va_heap> *references) } case IFN_MASK_LOAD: case IFN_MASK_STORE: + break; + case IFN_MASK_CALL: + { + tree orig_fndecl + = gimple_call_addr_fndecl (gimple_call_arg (stmt, 0)); + if (!orig_fndecl + || (flags_from_decl_or_type (orig_fndecl) & ECF_CONST) == 0) + clobbers_memory = true; + } break; default: clobbers_memory = true; @@ -5852,7 +5862,7 @@ get_references_in_stmt (gimple *stmt, vec<data_ref_loc, va_heap> *references) } else if (stmt_code == GIMPLE_CALL) { - unsigned i, n; + unsigned i = 0, n; tree ptr, type; unsigned int align; @@ -5879,13 +5889,16 @@ get_references_in_stmt (gimple *stmt, vec<data_ref_loc, va_heap> *references) ptr); references->safe_push (ref); return false; + case IFN_MASK_CALL: + i = 1; + gcc_fallthrough (); default: break; } op0 = gimple_call_lhs (stmt); n = gimple_call_num_args (stmt); - for (i = 0; i < n; i++) + for (; i < n; i++) { op1 = gimple_call_arg (stmt, i);
On 27/09/2023 08:56, Andre Vieira (lists) wrote: > > > On 26/09/2023 17:37, Andrew Stubbs wrote: >> I don't have authority to approve anything, but here's a review anyway. >> >> Thanks for working on this. > > Thank you for reviewing and apologies for the mess of a patch, may have > rushed it ;) >> >>> diff --git a/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c >>> b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c >>> new file mode 100644 >>> index >>> 0000000000000000000000000000000000000000..09127b8cb6f2e3699b6073591f58be7047330273 >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c >>> @@ -0,0 +1,23 @@ >>> +/* { dg-require-effective-target vect_simd_clones } */ >>> +/* { dg-do compile } */ >>> +/* { dg-additional-options "-fopenmp-simd" } */ >>> + >> >> Do you need -fopenmp-simd for this? > Nope, I keep forgetting that you only need it for pragmas. > > Dealt with the other comments too. > > Any thoughts on changing gimple_call_internal_fn instead? My main > argument against is that IFN_MASK_CALL should not appear outside of > ifconvert and vectorizer. On the other hand, we may inspect the flags > elsewhere in the vectorizer (now or in the future) and changing > gimple_call_internal_fn would prevent the need to handle the IFN > separately elsewhere. Sorry, I haven't looked closely enough to have an opinion on that, or what the side-effects might be. You have a solution, and like you said, this should be the only case. I have no further comments on this patch. :) Andrew
On Wed, 27 Sep 2023, Andre Vieira (lists) wrote: > > > On 26/09/2023 17:37, Andrew Stubbs wrote: > > I don't have authority to approve anything, but here's a review anyway. > > > > Thanks for working on this. > > Thank you for reviewing and apologies for the mess of a patch, may have rushed > it ;) > > > >> diff --git a/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c > >> b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c > >> new file mode 100644 > >> index > >> 0000000000000000000000000000000000000000..09127b8cb6f2e3699b6073591f58be7047330273 > >> --- /dev/null > >> +++ b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c > >> @@ -0,0 +1,23 @@ > >> +/* { dg-require-effective-target vect_simd_clones } */ > >> +/* { dg-do compile } */ > >> +/* { dg-additional-options "-fopenmp-simd" } */ > >> + > > > > Do you need -fopenmp-simd for this? > Nope, I keep forgetting that you only need it for pragmas. > > Dealt with the other comments too. The patch is OK. > Any thoughts on changing gimple_call_internal_fn instead? My main argument > against is that IFN_MASK_CALL should not appear outside of ifconvert and > vectorizer. On the other hand, we may inspect the flags elsewhere in the > vectorizer (now or in the future) and changing gimple_call_internal_fn would > prevent the need to handle the IFN separately elsewhere. But gimple_call_internal_fn is only half of the work since arguments are shifted. So I think handling this in if-conversion and the vectorizer is the right thing as it's a very short-lived IFN. Richard. > Kind Regards, > Andre >
diff --git a/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c new file mode 100644 index 0000000000000000000000000000000000000000..09127b8cb6f2e3699b6073591f58be7047330273 --- /dev/null +++ b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c @@ -0,0 +1,23 @@ +/* { dg-require-effective-target vect_simd_clones } */ +/* { dg-do compile } */ +/* { dg-additional-options "-fopenmp-simd" } */ + +int __attribute__ ((__simd__, const)) fn (int); + +void test (int * __restrict__ a, int * __restrict__ b, int n) +{ + for (int i = 0; i < n; ++i) + { + int a_; + if (b[i] > 0) + a_ = fn (b[i]); + else + a_ = b[i] + 5; + a[i] = a_; + } +} + +/* { dg-final { scan-tree-dump-not {loop contains function calls or data references} "vect" } } */ + +/* The LTO test produces two dump files and we scan the wrong one. */ +/* { dg-skip-if "" { *-*-* } { "-flto" } { "" } } */ diff --git a/gcc/tree-data-ref.cc b/gcc/tree-data-ref.cc index 6d3b7c2290e4db9c1168a4c763facb481157c97c..2926c3925ee7897fef53c16cfd1d19d23dbf05f3 100644 --- a/gcc/tree-data-ref.cc +++ b/gcc/tree-data-ref.cc @@ -100,6 +100,7 @@ along with GCC; see the file COPYING3. If not see #include "vr-values.h" #include "range-op.h" #include "tree-ssa-loop-ivopts.h" +#include "calls.h" static struct datadep_stats { @@ -5816,6 +5817,18 @@ get_references_in_stmt (gimple *stmt, vec<data_ref_loc, va_heap> *references) } case IFN_MASK_LOAD: case IFN_MASK_STORE: + case IFN_MASK_CALL: + { + tree orig_fndecl + = gimple_call_addr_fndecl (gimple_call_arg (stmt, 0)); + if (!orig_fndecl) + { + clobbers_memory = true; + break; + } + if ((flags_from_decl_or_type (orig_fndecl) & ECF_CONST) == 0) + clobbers_memory = true; + } break; default: clobbers_memory = true; @@ -5852,7 +5865,7 @@ get_references_in_stmt (gimple *stmt, vec<data_ref_loc, va_heap> *references) } else if (stmt_code == GIMPLE_CALL) { - unsigned i, n; + unsigned i = 0, n; tree ptr, type; unsigned int align; @@ -5879,13 +5892,15 @@ get_references_in_stmt (gimple *stmt, vec<data_ref_loc, va_heap> *references) ptr); references->safe_push (ref); return false; + case IFN_MASK_CALL: + i = 1; default: break; } op0 = gimple_call_lhs (stmt); n = gimple_call_num_args (stmt); - for (i = 0; i < n; i++) + for (; i < n; i++) { op1 = gimple_call_arg (stmt, i);
The const attribute is ignored when simdclone's are used inbranch. This is due to the fact that when analyzing a MASK_CALL we were not looking at the targeted function for flags, but instead only at the internal function call itself. This patch adds code to make sure we look at the target function to check for the const attribute and enables the autovectorization of inbranch const simdclones without needing the loop to be adorned the 'openmp simd' pragma. Not sure about how to add new includes to the ChangeLog. Which brings me to another point, I contemplated changing gimple_call_flags to do the checking of flags of the first argument of IFN_MASK_CALL itself rather than only calling internal_fn_flags on gimple_call_internal_fn (stmt), but that might be a bit too intrusive, opinions welcome :) Bootstrapped and regression tested on aarch64-unknown-linux-gnu and x86_64-pc-linux-gnu. Is this OK for trunk? gcc/ChangeLog: * tree-vect-data-ref.cc (include calls.h): Add new include. (get_references_in_stmt): Correctly handle IFN_MASK_CALL. gcc/testsuite/ChangeLog: * gcc.dg/vect/vect-simd-clone-19.c: New test.