| Message ID | 20240614125348.82C1F13AB1@imap1.dmz-prg2.suse.org |
|---|---|
| State | New |
| Headers | show |
| Series | Enhance if-conversion for automatic arrays | expand |
On Fri, Jun 14, 2024 at 5:54 AM Richard Biener <rguenther@suse.de> wrote: > > Automatic arrays that are not address-taken should not be subject to > store data races. That seems conservative enough. Though I would think if the array never escaped the function would be still correct and allow for more arrays (but maybe that is not tracked). Thanks, Andrew Pinski > This applies to OMP SIMD in-branch lowered > functions result array which for the testcase otherwise prevents > vectorization with SSE and for AVX and AVX512 ends up with spurious > .MASK_STORE to the stack surviving. > > This inefficiency was noted in PR111793. > > Bootstrap and regtest running on x86_64-unknown-linux-gnu. Is my > idea of store data races correct? At least phiopt uses the same > check but for example LIM doesn't special-case locals. > > PR tree-optimization/111793 > * tree-if-conv.cc (ifcvt_memrefs_wont_trap): For stores > that do not trap only consider -fstore-data-races when > the underlying object is not automatic or has its address > taken. > > * gcc.dg/vect/vect-simd-clone-21.c: New testcase. > --- > gcc/testsuite/gcc.dg/vect/vect-simd-clone-21.c | 16 ++++++++++++++++ > gcc/tree-if-conv.cc | 13 +++++++++++-- > 2 files changed, 27 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/vect/vect-simd-clone-21.c > > diff --git a/gcc/testsuite/gcc.dg/vect/vect-simd-clone-21.c b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-21.c > new file mode 100644 > index 00000000000..49c52fb59bd > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-21.c > @@ -0,0 +1,16 @@ > +/* { dg-do compile } */ > +/* { dg-require-effective-target vect_simd_clones } */ > +/* { dg-additional-options "-fopenmp-simd" } */ > + > +#pragma omp declare simd simdlen(4) inbranch > +__attribute__((noinline)) int > +foo (int a, int b) > +{ > + return a + b; > +} > + > +/* { dg-final { scan-tree-dump-times "vectorized 1 loops" 4 "vect" { target i?86-*-* x86_64-*-* } } } */ > +/* if-conversion shouldn't need to resort to masked stores for the result > + array created by OMP lowering since that's automatic and does not have > + its address taken. */ > +/* { dg-final { scan-tree-dump-not "MASK_STORE" "vect" } } */ > diff --git a/gcc/tree-if-conv.cc b/gcc/tree-if-conv.cc > index c4c3ed41a44..974c614edf3 100644 > --- a/gcc/tree-if-conv.cc > +++ b/gcc/tree-if-conv.cc > @@ -934,14 +934,23 @@ ifcvt_memrefs_wont_trap (gimple *stmt, vec<data_reference_p> drs) > if (DR_IS_READ (a)) > return true; > > + bool ok = flag_store_data_races; > + base = get_base_address (base); > + if (DECL_P (base) > + && auto_var_in_fn_p (base, cfun->decl) > + && ! may_be_aliased (base)) > + /* Automatic variables not aliased are not subject to > + data races. */ > + ok = true; > + > /* an unconditionaly write won't trap if the base is written > to unconditionally. */ > if (base_master_dr > && DR_BASE_W_UNCONDITIONALLY (*base_master_dr)) > - return flag_store_data_races; > + return ok; > /* or the base is known to be not readonly. */ > else if (base_object_writable (DR_REF (a))) > - return flag_store_data_races; > + return ok; > } > > return false; > -- > 2.35.3
On Fri, 14 Jun 2024, Andrew Pinski wrote: > On Fri, Jun 14, 2024 at 5:54 AM Richard Biener <rguenther@suse.de> wrote: > > > > Automatic arrays that are not address-taken should not be subject to > > store data races. > > That seems conservative enough. Though I would think if the array > never escaped the function would be still correct and allow for more > arrays (but maybe that is not tracked). points-to should have that computed (conservatively). I'll see to split out a ref_can_have_store_data_races (..) function. Richard. > Thanks, > Andrew Pinski > > > This applies to OMP SIMD in-branch lowered > > functions result array which for the testcase otherwise prevents > > vectorization with SSE and for AVX and AVX512 ends up with spurious > > .MASK_STORE to the stack surviving. > > > > This inefficiency was noted in PR111793. > > > > Bootstrap and regtest running on x86_64-unknown-linux-gnu. Is my > > idea of store data races correct? At least phiopt uses the same > > check but for example LIM doesn't special-case locals. > > > > PR tree-optimization/111793 > > * tree-if-conv.cc (ifcvt_memrefs_wont_trap): For stores > > that do not trap only consider -fstore-data-races when > > the underlying object is not automatic or has its address > > taken. > > > > * gcc.dg/vect/vect-simd-clone-21.c: New testcase. > > --- > > gcc/testsuite/gcc.dg/vect/vect-simd-clone-21.c | 16 ++++++++++++++++ > > gcc/tree-if-conv.cc | 13 +++++++++++-- > > 2 files changed, 27 insertions(+), 2 deletions(-) > > create mode 100644 gcc/testsuite/gcc.dg/vect/vect-simd-clone-21.c > > > > diff --git a/gcc/testsuite/gcc.dg/vect/vect-simd-clone-21.c b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-21.c > > new file mode 100644 > > index 00000000000..49c52fb59bd > > --- /dev/null > > +++ b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-21.c > > @@ -0,0 +1,16 @@ > > +/* { dg-do compile } */ > > +/* { dg-require-effective-target vect_simd_clones } */ > > +/* { dg-additional-options "-fopenmp-simd" } */ > > + > > +#pragma omp declare simd simdlen(4) inbranch > > +__attribute__((noinline)) int > > +foo (int a, int b) > > +{ > > + return a + b; > > +} > > + > > +/* { dg-final { scan-tree-dump-times "vectorized 1 loops" 4 "vect" { target i?86-*-* x86_64-*-* } } } */ > > +/* if-conversion shouldn't need to resort to masked stores for the result > > + array created by OMP lowering since that's automatic and does not have > > + its address taken. */ > > +/* { dg-final { scan-tree-dump-not "MASK_STORE" "vect" } } */ > > diff --git a/gcc/tree-if-conv.cc b/gcc/tree-if-conv.cc > > index c4c3ed41a44..974c614edf3 100644 > > --- a/gcc/tree-if-conv.cc > > +++ b/gcc/tree-if-conv.cc > > @@ -934,14 +934,23 @@ ifcvt_memrefs_wont_trap (gimple *stmt, vec<data_reference_p> drs) > > if (DR_IS_READ (a)) > > return true; > > > > + bool ok = flag_store_data_races; > > + base = get_base_address (base); > > + if (DECL_P (base) > > + && auto_var_in_fn_p (base, cfun->decl) > > + && ! may_be_aliased (base)) > > + /* Automatic variables not aliased are not subject to > > + data races. */ > > + ok = true; > > + > > /* an unconditionaly write won't trap if the base is written > > to unconditionally. */ > > if (base_master_dr > > && DR_BASE_W_UNCONDITIONALLY (*base_master_dr)) > > - return flag_store_data_races; > > + return ok; > > /* or the base is known to be not readonly. */ > > else if (base_object_writable (DR_REF (a))) > > - return flag_store_data_races; > > + return ok; > > } > > > > return false; > > -- > > 2.35.3 >
diff --git a/gcc/testsuite/gcc.dg/vect/vect-simd-clone-21.c b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-21.c new file mode 100644 index 00000000000..49c52fb59bd --- /dev/null +++ b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-21.c @@ -0,0 +1,16 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target vect_simd_clones } */ +/* { dg-additional-options "-fopenmp-simd" } */ + +#pragma omp declare simd simdlen(4) inbranch +__attribute__((noinline)) int +foo (int a, int b) +{ + return a + b; +} + +/* { dg-final { scan-tree-dump-times "vectorized 1 loops" 4 "vect" { target i?86-*-* x86_64-*-* } } } */ +/* if-conversion shouldn't need to resort to masked stores for the result + array created by OMP lowering since that's automatic and does not have + its address taken. */ +/* { dg-final { scan-tree-dump-not "MASK_STORE" "vect" } } */ diff --git a/gcc/tree-if-conv.cc b/gcc/tree-if-conv.cc index c4c3ed41a44..974c614edf3 100644 --- a/gcc/tree-if-conv.cc +++ b/gcc/tree-if-conv.cc @@ -934,14 +934,23 @@ ifcvt_memrefs_wont_trap (gimple *stmt, vec<data_reference_p> drs) if (DR_IS_READ (a)) return true; + bool ok = flag_store_data_races; + base = get_base_address (base); + if (DECL_P (base) + && auto_var_in_fn_p (base, cfun->decl) + && ! may_be_aliased (base)) + /* Automatic variables not aliased are not subject to + data races. */ + ok = true; + /* an unconditionaly write won't trap if the base is written to unconditionally. */ if (base_master_dr && DR_BASE_W_UNCONDITIONALLY (*base_master_dr)) - return flag_store_data_races; + return ok; /* or the base is known to be not readonly. */ else if (base_object_writable (DR_REF (a))) - return flag_store_data_races; + return ok; } return false;