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);