Re: [PATCH, OG10, OpenMP 5.0, committed] Remove array section base-pointer mapping semantics, and other front-end adjustments.
On 2021/5/11 11:15 , Thomas Schwinge wrote:
Hi Chung-Lin!
On 2021-05-11T19:28:04+0800, Chung-Lin Tang wrote:
This patch largely implements three pieces of functionality:
(1) Per discussion and clarification on the omp-lang mailing list,
standards conforming behavior for mapping array sections should *NOT* also map
the base-pointer,
i.e for this code:
struct S { int *ptr; ... };
struct S s;
#pragma omp target enter data map(to: s.ptr[:100])
Currently we generate after gimplify:
#pragma omp target enter data map(struct:s [len: 1]) map(alloc:s.ptr [len: 8]) \
map(to:*_1 [len: 400]) map(attach:s.ptr [bias:
0])
which is deemed incorrect. After this patch, the gimplify results are now
adjusted to:
#pragma omp target enter data map(to:*_1 [len: 400]) map(attach:s.ptr [bias: 0])
(the attach operation is still generated, and if s.ptr is already mapped prior,
attachment will happen)
The correct way of achieving the base-pointer-also-mapped behavior would be to
use:
#pragma omp target enter data map(to: s.ptr, s.ptr[:100])
This adjustment in behavior required a number of small adjustments here and
there in gimplify, including
to accomodate map sequences for C++ references.
I'm a bit confused by that -- this mandates the bulk of the testsuite
changes that you've included, and these seem a step backwards in terms of
user experience, but then, I have no state on the exact OpenMP
specification requirements, so you certainly may be right on that. (And
also, as Julian mentioned, how this relates to OpenACC semantics, which I
also haven't considered in detail -- but I note you didn't adjust any
OpenACC testcases for that, so I suppose that's really conditionalized to
OpenMP only.)
It is indeed a bit awkward to use, but that's what the omp-lang list seemed to
decide.
This change is OpenMP only. I took care to only handle OpenMP constructs like
this in the middle-end,
of course this does not preclude some mistake in adjusting the shared code
paths...
There is also a small Fortran front-end patch involved (hence CCing Tobias).
The new gimplify processing changed behavior in handling
GOMP_MAP_ALWAYS_POINTER maps such that
the libgomp.fortran/struct-elem-map-1.f90 regressed. It appeared that the
Fortran FE was generating
a GOMP_MAP_ALWAYS_POINTER for array types, which didn't seem quite correct, and
the pre-patch behavior
was removing this map anyways. I have a small change in
trans-openmp.c:gfc_trans_omp_array_section
to not generate the map in this case, and so far no bad test results.
Makes sense to argue that one separately, with testcases, for the master
branch submission?
Maybe. although this part was needed to solve a regression caused by the above
changes.
(2) The second part (though kind of related to the first above) are fixes in
libgomp/target.c
to not overwrite attached pointers when handling device<->host copies, mainly for the
"always" case.
This behavior is also noted in the 5.0 spec, but not yet properly coded before.
Likewise, if that makes sense?
Some of the separation of base-pointer/array-section in map clauses seemed to
step on this bug
(e.g. if one mechanically updates "s.ptr[:N]" into "s.ptr, s.ptr[:N]", and a
target-update overwrites the
base-pointer) So it's arguably separate, but also can cause some testsuite
chaos if not included together.
(3) The third is a set of changes to the C/C++ front-ends to extend the allowed
component access syntax
in map clauses. This is actually mainly an effort to allow SPEC HPC to compile,
so despite in the long
term the entire map clause syntax parsing is probably going to be revamped,
we're still adding this in
for now. These changes are enabled for both OpenACC and OpenMP.
Likewise, if that makes sense? ;-)
Yeah, this might be separated :P
Tested on x86_64-linux with nvptx offloading with no regressions.
I'm seeing a regression with
'libgomp.oacc-c-c++-common/noncontig_array-1.c' execution testing, both C
and C++, for '-O2' (but not '-O0'), and only for about half of the
invocations. But it seems to reliable reproduce in GDB:
Thread 1 "a.out" received signal SIGSEGV, Segmentation fault.
gomp_decrement_refcount (do_remove=, do_copy=, delete_p=false, refcount_set=0x0, k=0xc4d450) at
[...]/source-gcc/libgomp/target.c:468
468 uintptr_t orig_refcount = *refcount_ptr;
(gdb) bt
#0 gomp_decrement_refcount (do_remove=,
do_copy=, delete_p=false, refcount_set=0x0, k=0xc4d450) at
[...]/source-gcc/libgomp/target.c:468
#1 gomp_unmap_vars_internal (aq=0x0, aq@entry=0x8223c0, refcount_set=0x0,
do_copyfrom=, do_copyfrom@entry=true, tgt=tgt@entry=0xc696a0) at
[...]/source-gcc/libgomp/target.c:2065
#2 goacc_unmap_vars (tgt=tgt@entry=0xc696a0,
do_copyfrom=do_copyfrom@entry=true, aq=aq@entry=0x0) at
[...]/source-gcc/libgomp/target.c:2118
#3 0x77daa41c in GOACC_parallel_keyed (flags_m=flags_m@entry=-1,
Re: [PATCH, OG10, OpenMP 5.0, committed] Remove array section base-pointer mapping semantics, and other front-end adjustments.
Hi Chung-Lin!
On 2021-05-11T19:28:04+0800, Chung-Lin Tang wrote:
> This patch largely implements three pieces of functionality:
>
> (1) Per discussion and clarification on the omp-lang mailing list,
> standards conforming behavior for mapping array sections should *NOT* also
> map the base-pointer,
> i.e for this code:
>
> struct S { int *ptr; ... };
> struct S s;
> #pragma omp target enter data map(to: s.ptr[:100])
>
> Currently we generate after gimplify:
> #pragma omp target enter data map(struct:s [len: 1]) map(alloc:s.ptr [len:
> 8]) \
>map(to:*_1 [len: 400]) map(attach:s.ptr [bias:
> 0])
>
> which is deemed incorrect. After this patch, the gimplify results are now
> adjusted to:
> #pragma omp target enter data map(to:*_1 [len: 400]) map(attach:s.ptr [bias:
> 0])
> (the attach operation is still generated, and if s.ptr is already mapped
> prior, attachment will happen)
>
> The correct way of achieving the base-pointer-also-mapped behavior would be
> to use:
> #pragma omp target enter data map(to: s.ptr, s.ptr[:100])
>
> This adjustment in behavior required a number of small adjustments here and
> there in gimplify, including
> to accomodate map sequences for C++ references.
I'm a bit confused by that -- this mandates the bulk of the testsuite
changes that you've included, and these seem a step backwards in terms of
user experience, but then, I have no state on the exact OpenMP
specification requirements, so you certainly may be right on that. (And
also, as Julian mentioned, how this relates to OpenACC semantics, which I
also haven't considered in detail -- but I note you didn't adjust any
OpenACC testcases for that, so I suppose that's really conditionalized to
OpenMP only.)
> There is also a small Fortran front-end patch involved (hence CCing Tobias).
> The new gimplify processing changed behavior in handling
> GOMP_MAP_ALWAYS_POINTER maps such that
> the libgomp.fortran/struct-elem-map-1.f90 regressed. It appeared that the
> Fortran FE was generating
> a GOMP_MAP_ALWAYS_POINTER for array types, which didn't seem quite correct,
> and the pre-patch behavior
> was removing this map anyways. I have a small change in
> trans-openmp.c:gfc_trans_omp_array_section
> to not generate the map in this case, and so far no bad test results.
Makes sense to argue that one separately, with testcases, for the master
branch submission?
> (2) The second part (though kind of related to the first above) are fixes in
> libgomp/target.c
> to not overwrite attached pointers when handling device<->host copies, mainly
> for the "always" case.
> This behavior is also noted in the 5.0 spec, but not yet properly coded
> before.
Likewise, if that makes sense?
> (3) The third is a set of changes to the C/C++ front-ends to extend the
> allowed component access syntax
> in map clauses. This is actually mainly an effort to allow SPEC HPC to
> compile, so despite in the long
> term the entire map clause syntax parsing is probably going to be revamped,
> we're still adding this in
> for now. These changes are enabled for both OpenACC and OpenMP.
Likewise, if that makes sense? ;-)
> Tested on x86_64-linux with nvptx offloading with no regressions.
I'm seeing a regression with
'libgomp.oacc-c-c++-common/noncontig_array-1.c' execution testing, both C
and C++, for '-O2' (but not '-O0'), and only for about half of the
invocations. But it seems to reliable reproduce in GDB:
Thread 1 "a.out" received signal SIGSEGV, Segmentation fault.
gomp_decrement_refcount (do_remove=, do_copy=, delete_p=false, refcount_set=0x0, k=0xc4d450) at
[...]/source-gcc/libgomp/target.c:468
468 uintptr_t orig_refcount = *refcount_ptr;
(gdb) bt
#0 gomp_decrement_refcount (do_remove=,
do_copy=, delete_p=false, refcount_set=0x0, k=0xc4d450) at
[...]/source-gcc/libgomp/target.c:468
#1 gomp_unmap_vars_internal (aq=0x0, aq@entry=0x8223c0, refcount_set=0x0,
do_copyfrom=, do_copyfrom@entry=true, tgt=tgt@entry=0xc696a0) at
[...]/source-gcc/libgomp/target.c:2065
#2 goacc_unmap_vars (tgt=tgt@entry=0xc696a0,
do_copyfrom=do_copyfrom@entry=true, aq=aq@entry=0x0) at
[...]/source-gcc/libgomp/target.c:2118
#3 0x77daa41c in GOACC_parallel_keyed (flags_m=flags_m@entry=-1,
fn=fn@entry=0x400ae0 , mapnum=mapnum@entry=2,
hostaddrs=hostaddrs@entry=0x7fffd7a0, sizes=sizes@entry=0x604500
, kinds=kinds@entry=0x6044f0 ) at
[...]/source-gcc/libgomp/oacc-parallel.c:639
#4 0x00400f11 in test3 () at
source-gcc/libgomp/testsuite/libgomp.oacc-c/../libgomp.oacc-c-c++-common/noncontig_array-1.c:75
#5 0x004008f3 in main () at
source-gcc/libgomp/testsuite/libgomp.oacc-c/../libgomp.oacc-c-c++-common/noncontig_array-1.c:101
(gdb) print refcount_ptr
$1 = (uintptr_t *) 0x1
(gdb) list 457,468
457 uintptr_t *refcount_ptr = >refcount;
458
459 if (REFCOUNT_STRUCTELEM_FIRST_P (k->refcount))
460
Re: [PATCH, OG10, OpenMP 5.0, committed] Remove array section base-pointer mapping semantics, and other front-end adjustments.
On Tue, 11 May 2021 19:28:04 +0800
Chung-Lin Tang wrote:
> This patch largely implements three pieces of functionality:
>
> (1) Per discussion and clarification on the omp-lang mailing list,
> standards conforming behavior for mapping array sections should *NOT*
> also map the base-pointer, i.e for this code:
>
> struct S { int *ptr; ... };
> struct S s;
> #pragma omp target enter data map(to: s.ptr[:100])
>
> Currently we generate after gimplify:
> #pragma omp target enter data map(struct:s [len: 1]) map(alloc:s.ptr
> [len: 8]) \ map(to:*_1 [len: 400]) map(attach:s.ptr [bias: 0])
>
> which is deemed incorrect. After this patch, the gimplify results are
> now adjusted to: #pragma omp target enter data map(to:*_1 [len: 400])
> map(attach:s.ptr [bias: 0]) (the attach operation is still generated,
> and if s.ptr is already mapped prior, attachment will happen)
Oh, that's not going to play nicely (eventually?) with the patch series
I just posted... we probably need to clarify what the intention is for
OpenACC, but IIUC "user expectation" (i.e. existing code) expects the
base-pointer mapping to happen.
Julian
[PATCH, OG10, OpenMP 5.0, committed] Remove array section base-pointer mapping semantics, and other front-end adjustments.
This patch largely implements three pieces of functionality:
(1) Per discussion and clarification on the omp-lang mailing list,
standards conforming behavior for mapping array sections should *NOT* also map
the base-pointer,
i.e for this code:
struct S { int *ptr; ... };
struct S s;
#pragma omp target enter data map(to: s.ptr[:100])
Currently we generate after gimplify:
#pragma omp target enter data map(struct:s [len: 1]) map(alloc:s.ptr [len: 8]) \
map(to:*_1 [len: 400]) map(attach:s.ptr [bias: 0])
which is deemed incorrect. After this patch, the gimplify results are now
adjusted to:
#pragma omp target enter data map(to:*_1 [len: 400]) map(attach:s.ptr [bias: 0])
(the attach operation is still generated, and if s.ptr is already mapped prior,
attachment will happen)
The correct way of achieving the base-pointer-also-mapped behavior would be to
use:
#pragma omp target enter data map(to: s.ptr, s.ptr[:100])
This adjustment in behavior required a number of small adjustments here and
there in gimplify, including
to accomodate map sequences for C++ references.
There is also a small Fortran front-end patch involved (hence CCing Tobias).
The new gimplify processing changed behavior in handling
GOMP_MAP_ALWAYS_POINTER maps such that
the libgomp.fortran/struct-elem-map-1.f90 regressed. It appeared that the
Fortran FE was generating
a GOMP_MAP_ALWAYS_POINTER for array types, which didn't seem quite correct, and
the pre-patch behavior
was removing this map anyways. I have a small change in
trans-openmp.c:gfc_trans_omp_array_section
to not generate the map in this case, and so far no bad test results.
(2) The second part (though kind of related to the first above) are fixes in
libgomp/target.c
to not overwrite attached pointers when handling device<->host copies, mainly for the
"always" case.
This behavior is also noted in the 5.0 spec, but not yet properly coded before.
(3) The third is a set of changes to the C/C++ front-ends to extend the allowed
component access syntax
in map clauses. This is actually mainly an effort to allow SPEC HPC to compile,
so despite in the long
term the entire map clause syntax parsing is probably going to be revamped,
we're still adding this in
for now. These changes are enabled for both OpenACC and OpenMP.
Tested on x86_64-linux with nvptx offloading with no regressions. Pushed to
devel/omp/gcc-10, will
send mainline version of patch later.
Chung-Lin
2021-05-11 Chung-Lin Tang
gcc/c/ChangeLog:
* c-parser.c (struct omp_dim): New struct type for use inside
c_parser_omp_variable_list.
(c_parser_omp_variable_list): Allow multiple levels of array and
component accesses in array section base-pointer expression.
(c_parser_omp_clause_to): Set 'allow_deref' to true in call to
c_parser_omp_var_list_parens.
(c_parser_omp_clause_from): Likewise.
* c-typeck.c (handle_omp_array_sections_1): Extend allowed range
of base-pointer expressions involving INDIRECT/MEM/ARRAY_REF and
POINTER_PLUS_EXPR.
(c_finish_omp_clauses): Extend allowed ranged of expressions
involving INDIRECT/MEM/ARRAY_REF and POINTER_PLUS_EXPR.
gcc/cp/ChangeLog:
* parser.c (struct omp_dim): New struct type for use inside
cp_parser_omp_var_list_no_open.
(cp_parser_omp_var_list_no_open): Allow multiple levels of array and
component accesses in array section base-pointer expression.
(cp_parser_omp_all_clauses): Set 'allow_deref' to true in call to
cp_parser_omp_var_list for to/from clauses.
* semantics.c (handle_omp_array_sections_1): Extend allowed range
of base-pointer expressions involving INDIRECT/MEM/ARRAY_REF and
POINTER_PLUS_EXPR.
(handle_omp_array_sections): Adjust pointer map generation of
references.
(finish_omp_clauses): Extend allowed ranged of expressions
involving INDIRECT/MEM/ARRAY_REF and POINTER_PLUS_EXPR.
gcc/fortran/ChangeLog:
* trans-openmp.c (gfc_trans_omp_array_section): Do not generate
GOMP_MAP_ALWAYS_POINTER map for main array maps of ARRAY_TYPE type.
gcc/ChangeLog:
* gimplify.c (extract_base_bit_offset): Add 'tree *offsetp' parameter,
accomodate case where 'offset' return of get_inner_reference is
non-NULL.
(is_or_contains_p): Further robustify conditions.
(omp_target_reorder_clauses): In alloc/to/from sorting phase, also
move following GOMP_MAP_ALWAYS_POINTER maps along. Add new sorting
phase where we make sure pointers with an attach/detach map are ordered
correctly.
(gimplify_scan_omp_clauses): Add modifications to avoid creating
GOMP_MAP_STRUCT and associated alloc map for attach/detach maps.
gcc/testsuite/ChangeLog:
* c-c++-common/goacc/deep-copy-arrayofstruct.c: Adjust testcase.
* c-c++-common/gomp/target-enter-data-1.c:
