On Fri, 13 Dec 2019 15:13:53 +0100
Thomas Schwinge <tho...@codesourcery.com> wrote:
> Hi!
>
> Julian, Tobias, regarding the following OpenACC 'exit data' 'finalize'
> handling:
>
> On 2018-05-25T13:01:58-0700, Cesar Philippidis
> <ce...@codesourcery.com> wrote:
> > --- a/gcc/gimplify.c
> > +++ b/gcc/gimplify.c
>
> > @@ -10859,6 +10849,53 @@ gimplify_omp_target_update (tree *expr_p,
> > gimple_seq *pre_p)
>
> > + else if (TREE_CODE (expr) == OACC_EXIT_DATA
> > + && omp_find_clause (OMP_STANDALONE_CLAUSES (expr),
> > + OMP_CLAUSE_FINALIZE))
> > + {
> > + /* Use GOMP_MAP_DELETE/GOMP_MAP_FORCE_FROM to denote that
> > "finalize"
> > + semantics apply to all mappings of this OpenACC
> > directive. */
> > + bool finalize_marked = false;
> > + for (tree c = OMP_STANDALONE_CLAUSES (expr); c; c =
> > OMP_CLAUSE_CHAIN (c))
> > + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP)
> > + switch (OMP_CLAUSE_MAP_KIND (c))
> > + {
> > + case GOMP_MAP_FROM:
> > + OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_FORCE_FROM);
> > + finalize_marked = true;
> > + break;
> > + case GOMP_MAP_RELEASE:
> > + OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_DELETE);
> > + finalize_marked = true;
> > + break;
> > + default:
> > + /* Check consistency: libgomp relies on the very
> > first data
> > + mapping clause being marked, so make sure we did
> > that before
> > + any other mapping clauses. */
> > + gcc_assert (finalize_marked);
> > + break;
> > + }
> > + }
>
> > --- a/libgomp/oacc-parallel.c
> > +++ b/libgomp/oacc-parallel.c
>
> > @@ -286,6 +360,17 @@ GOACC_enter_exit_data (int device, size_t
> > mapnum,
>
> > + /* Determine whether "finalize" semantics apply to all mappings
> > of this
> > + OpenACC directive. */
> > + bool finalize = false;
> > + if (mapnum > 0)
> > + {
> > + unsigned char kind = kinds[0] & 0xff;
> > + if (kind == GOMP_MAP_DELETE
> > + || kind == GOMP_MAP_FORCE_FROM)
> > + finalize = true;
> > + }
> > +
>
> > @@ -360,22 +458,28 @@ GOACC_enter_exit_data (int device, size_t
> > mapnum,
>
> > switch (kind)
> > {
> > - case GOMP_MAP_POINTER:
> > - gomp_acc_remove_pointer (hostaddrs[i], (kinds[i] &
> > 0xff)
> > - == GOMP_MAP_FORCE_FROM,
> > - async, 1);
> > - break;
> > + case GOMP_MAP_RELEASE:
> > case GOMP_MAP_DELETE:
> > - acc_delete (hostaddrs[i], sizes[i]);
> > + if (acc_is_present (hostaddrs[i], sizes[i]))
> > + {
> > + if (finalize)
> > + acc_delete_finalize (hostaddrs[i], sizes[i]);
> > + else
> > + acc_delete (hostaddrs[i], sizes[i]);
> > + }
> > break;
> > + case GOMP_MAP_FROM:
> > case GOMP_MAP_FORCE_FROM:
> > - acc_copyout (hostaddrs[i], sizes[i]);
> > + if (finalize)
> > + acc_copyout_finalize (hostaddrs[i], sizes[i]);
> > + else
> > + acc_copyout (hostaddrs[i], sizes[i]);
> > break;
> > default:
> > gomp_fatal (">>>> GOACC_enter_exit_data UNHANDLED
> > kind 0x%.2x", @@ -385,10 +489,12 @@ GOACC_enter_exit_data (int
> > device, size_t mapnum, }
> > else
> > {
> > - gomp_acc_remove_pointer (hostaddrs[i], (kinds[i] &
> > 0xff)
> > - == GOMP_MAP_FORCE_FROM,
> > async, 3); +[...]
> > + gomp_acc_remove_pointer (hostaddrs[i], sizes[i],
> > copyfrom, async,
> > + finalize, pointer);
>
> ... does the attached patch "[OpenACC] Elaborate/simplify 'exit data'
> 'finalize' handling" (with "No functional changes") match your
> understanding of what's going on?
Your patch looks OK to me, FWIW. As you mentioned at some point though,
it might be good to get rid of this style of finalize handling,
replacing it with a flag passed to GOACC_exit_data -- presuming that at
the same time, we separate out the needlessly-dual-purpose
GOACC_enter_exit_data API entry point into "enter" and "exit" halves.
Thanks,
Julian
