| Message ID | b82edc84ada1c5d260100cf913da5ce0a2f15583.1592343756.git.julian@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | Refcounting and manual deep copy improvements | expand |
Hi Julian! On 2020-06-16T15:39:42-0700, Julian Brown <julian@codesourcery.com> wrote: > This is a fix for the pointer (or array) size inadvertently being used > for the bias of attach and detach clauses (PR95270) Thanks for looking into that one, which had caused my some gray hair. > for C and C++. That means, there is no such problem for Fortran? (I haven't run into one, just curious.) > OK? In principle, yes, for master and releases/gcc-10 branches, but please incorporate the following items: > PR middle-end/95270 > > gcc/c/ > * c-typeck.c (c_finish_omp_clauses): Set OMP_CLAUSE_SIZE (bias) to zero > for standalone attach/detach clauses. > > gcc/cp/ > * semantics.c (finish_omp_clauses): Likewise. > > gcc/testsuite/ > * c-c++-common/goacc/mdc-1.c: Update expected dump output for zero > bias. > --- > gcc/c/c-typeck.c | 8 ++++++++ > gcc/cp/semantics.c | 8 ++++++++ > gcc/testsuite/c-c++-common/goacc/mdc-1.c | 14 +++++++------- > 3 files changed, 23 insertions(+), 7 deletions(-) > --- a/gcc/c/c-typeck.c > +++ b/gcc/c/c-typeck.c > @@ -14533,6 +14533,10 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > } > if (c_oacc_check_attachments (c)) > remove = true; > + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP > + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH > + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH)) > + OMP_CLAUSE_SIZE (c) = size_zero_node; > break; > } > if (t == error_mark_node) > @@ -14546,6 +14550,10 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > remove = true; > break; > } > + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP > + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH > + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH)) > + OMP_CLAUSE_SIZE (c) = size_zero_node; > if (TREE_CODE (t) == COMPONENT_REF > && OMP_CLAUSE_CODE (c) != OMP_CLAUSE__CACHE_) > { I cannot comment if these two code paths are good places (and the only ones) that need to set 'OMP_CLAUSE_SIZE', so I'll trust you've found the best/all places. Does that override an 'OMP_CLAUSE_SIZE' that was set earlier, or initialize it? Maybe the latter, given my comment in <https://gcc.gnu.org/PR95270>: "make sure to skip/invalidate the 'gcc/gimplify.c:gimplify_scan_omp_clauses' handling"? Plase add some commentary here in the code, instead of just in the ChangeLog, something like: "initialize here, so that gimplify doesn't wrongly do so later" (if that's what it is, and in proper language, of course). > --- a/gcc/cp/semantics.c > +++ b/gcc/cp/semantics.c > @@ -7334,6 +7334,10 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > } > if (cp_oacc_check_attachments (c)) > remove = true; > + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP > + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH > + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH)) > + OMP_CLAUSE_SIZE (c) = size_zero_node; > break; > } > if (t == error_mark_node) > @@ -7347,6 +7351,10 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > remove = true; > break; > } > + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP > + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH > + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH)) > + OMP_CLAUSE_SIZE (c) = size_zero_node; > if (REFERENCE_REF_P (t) > && TREE_CODE (TREE_OPERAND (t, 0)) == COMPONENT_REF) > { Likewise. > --- a/gcc/testsuite/c-c++-common/goacc/mdc-1.c > +++ b/gcc/testsuite/c-c++-common/goacc/mdc-1.c Obvious. In <https://gcc.gnu.org/PR95270> I also requested size vs. bias be documented in 'include/gomp-constants.h:enum gomp_map_kind'. Generally, I'm still somewhat confused by the 'bias' usage in libgomp. Is it really only used for the *initial* attach, but then (correctly so?) ignored for any later ones? Please add some commentary next to the respective libgomp code. Please also include an execution test case, like I had included with <https://gcc.gnu.org/PR95270>, for example the two files I'm attaching. Ah actually, since the directive variant now no longer fails, please merge these into one file, with 'test(bool directive)', and two 'test(false)', 'test(true)' calls from 'main'. Grüße Thomas ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Hi Julian! On 2020-06-25T13:36:15+0200, I wrote: > On 2020-06-16T15:39:42-0700, Julian Brown <julian@codesourcery.com> wrote: >> This is a fix for the pointer (or array) size inadvertently being used >> for the bias of attach and detach clauses (PR95270) > > Thanks for looking into that one, which had caused my some gray hair. > >> for C and C++. > > That means, there is no such problem for Fortran? (I haven't run into > one, just curious.) Looking into something else, I've now found the very same (?) problem for Fortran, too. :-| For the following simple testcase, I again do see non-zero 'bias: 64' for 'enter data attach(data_p)': program main use openacc implicit none !TODO Per PR96080, data types chosen so that we can create a "pointer object 'data_p'" on the device. integer, dimension(:), target :: data(1) integer, dimension(:), pointer :: data_p !TODO Per PR96080, not using OpenACC/Fortran runtime library routines. !$acc enter data create(data) data_p => data !$acc enter data copyin(data_p) !$acc enter data attach(data_p) end program main ..., and the 'attach' fails with 'libgomp: pointer target not mapped for attach'. It doesn't fail when I force 'bias = 0' in 'gomp_attach_pointer'. I've tried a bit, but it seems a bit difficult in Fortran to verify (with 'associated(data_p, data)' etc.) what we've actually 'attach'ed: per PR96080, a 'call acc_update_self(data_p)' may not be doing the expected thing, and a '!$acc update self(data_p)' per 'libgomp/oacc-parallel.c:GOACC_update' will update the actual data, but is no-op for 'GOMP_MAP_TO_PSET', 'GOMP_MAP_POINTER'. I've stopped experimenting with that any further. So it seems Fortran front end changes will also be required in addition to the C, C++ front end changes you've come up with. (For avoidance of doubt: OK to do separately, if you'd like to. Please also reference GCC PR95270 for these, and include the testcase from above, or something better.) Grüße Thomas > In principle, yes, for master and releases/gcc-10 branches, but please > incorporate the following items: > >> PR middle-end/95270 >> >> gcc/c/ >> * c-typeck.c (c_finish_omp_clauses): Set OMP_CLAUSE_SIZE (bias) to zero >> for standalone attach/detach clauses. >> >> gcc/cp/ >> * semantics.c (finish_omp_clauses): Likewise. >> >> gcc/testsuite/ >> * c-c++-common/goacc/mdc-1.c: Update expected dump output for zero >> bias. >> --- >> gcc/c/c-typeck.c | 8 ++++++++ >> gcc/cp/semantics.c | 8 ++++++++ >> gcc/testsuite/c-c++-common/goacc/mdc-1.c | 14 +++++++------- >> 3 files changed, 23 insertions(+), 7 deletions(-) > >> --- a/gcc/c/c-typeck.c >> +++ b/gcc/c/c-typeck.c >> @@ -14533,6 +14533,10 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> } >> if (c_oacc_check_attachments (c)) >> remove = true; >> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP >> + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH >> + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH)) >> + OMP_CLAUSE_SIZE (c) = size_zero_node; >> break; >> } >> if (t == error_mark_node) >> @@ -14546,6 +14550,10 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> remove = true; >> break; >> } >> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP >> + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH >> + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH)) >> + OMP_CLAUSE_SIZE (c) = size_zero_node; >> if (TREE_CODE (t) == COMPONENT_REF >> && OMP_CLAUSE_CODE (c) != OMP_CLAUSE__CACHE_) >> { > > I cannot comment if these two code paths are good places (and the only > ones) that need to set 'OMP_CLAUSE_SIZE', so I'll trust you've found the > best/all places. > > Does that override an 'OMP_CLAUSE_SIZE' that was set earlier, or > initialize it? Maybe the latter, given my comment in > <https://gcc.gnu.org/PR95270>: "make sure to skip/invalidate the > 'gcc/gimplify.c:gimplify_scan_omp_clauses' handling"? > > Plase add some commentary here in the code, instead of just in the > ChangeLog, something like: "initialize here, so that gimplify doesn't > wrongly do so later" (if that's what it is, and in proper language, of > course). > >> --- a/gcc/cp/semantics.c >> +++ b/gcc/cp/semantics.c >> @@ -7334,6 +7334,10 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> } >> if (cp_oacc_check_attachments (c)) >> remove = true; >> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP >> + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH >> + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH)) >> + OMP_CLAUSE_SIZE (c) = size_zero_node; >> break; >> } >> if (t == error_mark_node) >> @@ -7347,6 +7351,10 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> remove = true; >> break; >> } >> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP >> + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH >> + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH)) >> + OMP_CLAUSE_SIZE (c) = size_zero_node; >> if (REFERENCE_REF_P (t) >> && TREE_CODE (TREE_OPERAND (t, 0)) == COMPONENT_REF) >> { > > Likewise. > >> --- a/gcc/testsuite/c-c++-common/goacc/mdc-1.c >> +++ b/gcc/testsuite/c-c++-common/goacc/mdc-1.c > > Obvious. > > In <https://gcc.gnu.org/PR95270> I also requested size vs. bias be > documented in 'include/gomp-constants.h:enum gomp_map_kind'. > > Generally, I'm still somewhat confused by the 'bias' usage in libgomp. > Is it really only used for the *initial* attach, but then (correctly so?) > ignored for any later ones? Please add some commentary next to the > respective libgomp code. > > Please also include an execution test case, like I had included with > <https://gcc.gnu.org/PR95270>, for example the two files I'm attaching. > Ah actually, since the directive variant now no longer fails, please > merge these into one file, with 'test(bool directive)', and two > 'test(false)', 'test(true)' calls from 'main'. > > > Grüße > Thomas > [ pr95270_-d.c: text/x-csrc ] > #define DIRECTIVE > #include "pr95270_-r.c" > [ pr95270_-r.c: text/x-csrc ] > // <https://gcc.gnu.org/PR95270> > > #include <assert.h> > #include <openacc.h> > > int main() > { > int data; > int *data_p_dev = (int *) acc_create(&data, sizeof data); > int *data_p = &data; > acc_copyin(&data_p, sizeof data_p); > > #ifdef DIRECTIVE > # pragma acc enter data attach(data_p) > #else > { > void **ptr = (void **) &data_p; > acc_attach(ptr); > } > #endif > > acc_update_self(&data_p, sizeof data_p); > assert (data_p == data_p_dev); > > return 0; > } ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
On Thu, 9 Jul 2020 23:06:29 +0200 Thomas Schwinge <thomas@codesourcery.com> wrote: > Hi Julian! > > On 2020-06-25T13:36:15+0200, I wrote: > > On 2020-06-16T15:39:42-0700, Julian Brown <julian@codesourcery.com> > > wrote: > >> This is a fix for the pointer (or array) size inadvertently being > >> used for the bias of attach and detach clauses (PR95270) > > > > Thanks for looking into that one, which had caused my some gray > > hair. > >> for C and C++. > > > > That means, there is no such problem for Fortran? (I haven't run > > into one, just curious.) > > Looking into something else, I've now found the very same (?) problem > for Fortran, too. :-| For the following simple testcase, I again do > see non-zero 'bias: 64' for 'enter data attach(data_p)': > > program main > use openacc > implicit none > !TODO Per PR96080, data types chosen so that we can create a > "pointer object 'data_p'" on the device. integer, dimension(:), > target :: data(1) integer, dimension(:), pointer :: data_p > > !TODO Per PR96080, not using OpenACC/Fortran runtime library > routines. > !$acc enter data create(data) > data_p => data > !$acc enter data copyin(data_p) > > !$acc enter data attach(data_p) > end program main > > ..., and the 'attach' fails with 'libgomp: pointer target not mapped > for attach'. It doesn't fail when I force 'bias = 0' in > 'gomp_attach_pointer'. > > I've tried a bit, but it seems a bit difficult in Fortran to verify > (with 'associated(data_p, data)' etc.) what we've actually > 'attach'ed: per PR96080, a 'call acc_update_self(data_p)' may not be > doing the expected thing, and a '!$acc update self(data_p)' per > 'libgomp/oacc-parallel.c:GOACC_update' will update the actual data, > but is no-op for 'GOMP_MAP_TO_PSET', 'GOMP_MAP_POINTER'. I've stopped > experimenting with that any further. > > So it seems Fortran front end changes will also be required in > addition to the C, C++ front end changes you've come up with. (For > avoidance of doubt: OK to do separately, if you'd like to. Please > also reference GCC PR95270 for these, and include the testcase from > above, or something better.) Do the 7th & 8th patches in this series help? They were "supposed to" be the Fortran equivalent of these C/C++ changes, though I found additional problems too. Thanks, Julian
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index 385bf3a1c7b..134f1520239 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -14533,6 +14533,10 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort) } if (c_oacc_check_attachments (c)) remove = true; + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH)) + OMP_CLAUSE_SIZE (c) = size_zero_node; break; } if (t == error_mark_node) @@ -14546,6 +14550,10 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort) remove = true; break; } + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH)) + OMP_CLAUSE_SIZE (c) = size_zero_node; if (TREE_CODE (t) == COMPONENT_REF && OMP_CLAUSE_CODE (c) != OMP_CLAUSE__CACHE_) { diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c index 64587c791c6..77e6ff7fb0d 100644 --- a/gcc/cp/semantics.c +++ b/gcc/cp/semantics.c @@ -7334,6 +7334,10 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) } if (cp_oacc_check_attachments (c)) remove = true; + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH)) + OMP_CLAUSE_SIZE (c) = size_zero_node; break; } if (t == error_mark_node) @@ -7347,6 +7351,10 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) remove = true; break; } + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH)) + OMP_CLAUSE_SIZE (c) = size_zero_node; if (REFERENCE_REF_P (t) && TREE_CODE (TREE_OPERAND (t, 0)) == COMPONENT_REF) { diff --git a/gcc/testsuite/c-c++-common/goacc/mdc-1.c b/gcc/testsuite/c-c++-common/goacc/mdc-1.c index fb5841a709d..337c1f7cc77 100644 --- a/gcc/testsuite/c-c++-common/goacc/mdc-1.c +++ b/gcc/testsuite/c-c++-common/goacc/mdc-1.c @@ -45,12 +45,12 @@ t1 () /* { dg-final { scan-tree-dump-times "pragma omp target oacc_enter_exit_data map.to:s .len: 32.." 1 "omplower" } } */ /* { dg-final { scan-tree-dump-times "pragma omp target oacc_data map.tofrom:.z .len: 40.. map.struct:s .len: 1.. map.alloc:s.a .len: 8.. map.tofrom:._1 .len: 40.. map.attach:s.a .bias: 0.." 1 "omplower" } } */ -/* { dg-final { scan-tree-dump-times "pragma omp target oacc_parallel map.attach:s.e .bias: 8.. map.tofrom:s .len: 32" 1 "omplower" } } */ -/* { dg-final { scan-tree-dump-times "pragma omp target oacc_enter_exit_data map.attach:a .bias: 8.." 1 "omplower" } } */ -/* { dg-final { scan-tree-dump-times "pragma omp target oacc_enter_exit_data map.detach:a .bias: 8.." 1 "omplower" } } */ +/* { dg-final { scan-tree-dump-times "pragma omp target oacc_parallel map.attach:s.e .bias: 0.. map.tofrom:s .len: 32" 1 "omplower" } } */ +/* { dg-final { scan-tree-dump-times "pragma omp target oacc_enter_exit_data map.attach:a .bias: 0.." 1 "omplower" } } */ +/* { dg-final { scan-tree-dump-times "pragma omp target oacc_enter_exit_data map.detach:a .bias: 0.." 1 "omplower" } } */ /* { dg-final { scan-tree-dump-times "pragma omp target oacc_enter_exit_data map.to:a .len: 8.." 1 "omplower" } } */ -/* { dg-final { scan-tree-dump-times "pragma omp target oacc_enter_exit_data map.detach:s.e .bias: 8.." 1 "omplower" } } */ -/* { dg-final { scan-tree-dump-times "pragma omp target oacc_data map.attach:s.e .bias: 8.." 1 "omplower" } } */ +/* { dg-final { scan-tree-dump-times "pragma omp target oacc_enter_exit_data map.detach:s.e .bias: 0.." 1 "omplower" } } */ +/* { dg-final { scan-tree-dump-times "pragma omp target oacc_data map.attach:s.e .bias: 0.." 1 "omplower" } } */ /* { dg-final { scan-tree-dump-times "pragma omp target oacc_enter_exit_data map.release:a .len: 8.." 1 "omplower" } } */ -/* { dg-final { scan-tree-dump-times "pragma omp target oacc_enter_exit_data finalize map.force_detach:a .bias: 8.." 1 "omplower" } } */ -/* { dg-final { scan-tree-dump-times "pragma omp target oacc_enter_exit_data finalize map.force_detach:s.a .bias: 8.." 1 "omplower" } } */ +/* { dg-final { scan-tree-dump-times "pragma omp target oacc_enter_exit_data finalize map.force_detach:a .bias: 0.." 1 "omplower" } } */ +/* { dg-final { scan-tree-dump-times "pragma omp target oacc_enter_exit_data finalize map.force_detach:s.a .bias: 0.." 1 "omplower" } } */