Hi!
Jakub, please see one question below.
On 2019-12-24T15:23:56+0100, Tobias Burnus <tob...@codesourcery.com> wrote:
> OK for the trunk?
Tobias, thanks for taking over this patch. I have a few additional small
changes that I'd like to do (have WIP patches already), but what we've
got here already is OK for trunk with minor changes, see below.
> PS: History: [...]
> A minor fix was committed to OG8?/OG9 as Rev.
> 995f9680a46c3a7246fe465faa847f8009e47ed8.
That patch by Julian is on og9 only, and -- as far as I can tell -- has
never been posted/discussed. It may be obvious enough; we shall look
into it later. (I have not yet verified whether it is sufficiently
contained in the patch you posted here.)
> --- a/gcc/fortran/openmp.c
> +++ b/gcc/fortran/openmp.c
> -#define OACC_HOST_DATA_CLAUSES omp_mask (OMP_CLAUSE_USE_DEVICE)
> +#define OACC_HOST_DATA_CLAUSES omp_mask \
Please remove the superfluous (benign) 'omp_mask' on the line above.
> + (omp_mask (OMP_CLAUSE_USE_DEVICE) \
> + | OMP_CLAUSE_IF \
> + | OMP_CLAUSE_IF_PRESENT)
> --- a/gcc/omp-low.c
> +++ b/gcc/omp-low.c
> @@ -12006,6 +12006,9 @@ lower_omp_target (gimple_stmt_iterator *gsi_p,
> omp_context *ctx)
| case OMP_CLAUSE_USE_DEVICE_PTR:
| case OMP_CLAUSE_USE_DEVICE_ADDR:
| case OMP_CLAUSE_IS_DEVICE_PTR:
| ovar = OMP_CLAUSE_DECL (c);
| var = lookup_decl_in_outer_ctx (ovar, ctx);
|
| if (lang_hooks.decls.omp_array_data (ovar, true))
| {
| tkind = (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_IS_DEVICE_PTR
| ? GOMP_MAP_USE_DEVICE_PTR : GOMP_MAP_FIRSTPRIVATE_INT);
| x = build_sender_ref ((splay_tree_key) &DECL_NAME (ovar), ctx);
| }
| else if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_IS_DEVICE_PTR)
| {
| tkind = GOMP_MAP_USE_DEVICE_PTR;
| x = build_sender_ref ((splay_tree_key) &DECL_UID (ovar), ctx);
| }
| else
| {
> tkind = GOMP_MAP_FIRSTPRIVATE_INT;
> x = build_sender_ref (ovar, ctx);
> }
> + if (tkind == GOMP_MAP_USE_DEVICE_PTR
> + && omp_find_clause (clauses, OMP_CLAUSE_IF_PRESENT))
> + tkind = GOMP_MAP_USE_DEVICE_PTR_IF_PRESENT;
(For context: OpenACC only has an 'use_device' clause, mapped to
'OMP_CLAUSE_USE_DEVICE_PTR'; the other ones are for OpenMP only.)
Can you foresee any C/C++/Fortran "magic" ;-) such that for OpenACC
'host_data' construct with 'use_device' clause(s), we'd get something
different from 'tkind == GOMP_MAP_USE_DEVICE_PTR' here? In that case,
any 'if_present' clause would not be effective. (I have a WIP patch to
'gcc_assert' that.)
> type = TREE_TYPE (ovar);
> if (lang_hooks.decls.omp_array_data (ovar, true))
> var = lang_hooks.decls.omp_array_data (ovar, false);
> --- a/include/gomp-constants.h
> +++ b/include/gomp-constants.h
> @@ -93,6 +93,10 @@ enum gomp_map_kind
> at the address. If not already mapped, do nothing (and pointer
> translate
> to NULL). */
> GOMP_MAP_ZERO_LEN_ARRAY_SECTION = (GOMP_MAP_FLAG_SPECIAL | 3),
> + /* Like GOMP_MAP_USE_DEVICE_PTR below, translate a host to a device
'GOMP_MAP_USE_DEVICE_PTR' is "above", not "below". ;-)
> + address. If translation fails because the target is not mapped,
> + continue using the host address. */
> + GOMP_MAP_USE_DEVICE_PTR_IF_PRESENT = (GOMP_MAP_FLAG_SPECIAL_2 | 0),
Move 'GOMP_MAP_USE_DEVICE_PTR_IF_PRESENT' after the 'GOMP_MAP_FORCE_*'
ones following here:
> /* Allocate. */
> GOMP_MAP_FORCE_ALLOC = (GOMP_MAP_FLAG_FORCE | GOMP_MAP_ALLOC),
> /* ..., and copy to device. */
| GOMP_MAP_FORCE_TO = (GOMP_MAP_FLAG_FORCE |
GOMP_MAP_TO),
| /* ..., and copy from device. */
| GOMP_MAP_FORCE_FROM = (GOMP_MAP_FLAG_FORCE | GOMP_MAP_FROM),
| /* ..., and copy to and from device. */
| GOMP_MAP_FORCE_TOFROM = (GOMP_MAP_FLAG_FORCE | GOMP_MAP_TOFROM),
..., so right before the 'GOMP_MAP_ALWAYS_*' ones, which are the other
'GOMP_MAP_FLAG_SPECIAL_2' users:
| /* If not already present, allocate. And unconditionally copy to
| device. */
| GOMP_MAP_ALWAYS_TO = (GOMP_MAP_FLAG_SPECIAL_2 | GOMP_MAP_TO),
| /* If not already present, allocate. And unconditionally copy from
| device. */
| GOMP_MAP_ALWAYS_FROM = (GOMP_MAP_FLAG_SPECIAL_2
| | GOMP_MAP_FROM),
| /* If not already present, allocate. And unconditionally copy to and
from
| device. */
| GOMP_MAP_ALWAYS_TOFROM = (GOMP_MAP_FLAG_SPECIAL_2
| | GOMP_MAP_TOFROM),
Jakub, please speak up in case that it's not OK to occupy
'GOMP_MAP_FLAG_SPECIAL_2 | 0' for 'GOMP_MAP_USE_DEVICE_PTR_IF_PRESENT',
if you foresee any other/better use of that value. This value would
correspond to (non-sensical) "'GOMP_MAP_ALWAYS_ALLOC'".
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
I'm have a WIP patch that should make this even more simpler.
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/host_data-7.c
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/host_data-5.F90
If I understand correctly that these two are testing the same things,
then please cross-reference them: "C/C++ variant of
'libgomp.oacc-fortran/host_data-5.F90'", "Fortran variant of
'libgomp.oacc-c-c++-common/host_data-7.c'.
If that makes sense to consider (now or later), as I see there's some
overlap in the 'gcc/omp-low.c:lower_omp_target' code paths: do we need
test cases to verify 'if_present' in combination with Fortran optional
arguments?
Grüße
Thomas
