Bug 92929 - OpenACC/OpenMP 'target' 'exit data'/'update' optimizations
Summary: OpenACC/OpenMP 'target' 'exit data'/'update' optimizations
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: unknown
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: openacc, openmp
Depends on: 94635
Blocks:
  Show dependency treegraph
 
Reported: 2019-12-13 10:56 UTC by Thomas Schwinge
Modified: 2022-01-09 00:34 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2020-07-06 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Schwinge 2019-12-13 10:56:29 UTC
As of Tobias' recent r277631 "Fortran/OpenMP] Don't create "alloc:" for 'target exit data'", <http://mid.mail-archive.com/7a5f39e8-a33b-048a-f9c1-1355b941771e@codesourcery.com>, this is done for Fortran OpenMP '!$omp target exit data'/'!$omp target update', but not for OpenACC '!$acc exit data'/'!$acc update'; see 'gcc/gimplify.c:gimplify_scan_omp_clauses':

    /* For Fortran, not only the pointer to the data is mapped but also
       the address of the pointer, the array descriptor etc.; for
       'exit data' - and in particular for 'delete:' - having an 'alloc:'
       does not make sense.  Likewise, for 'update' only transferring the
       data itself is needed as the rest has been handled in previous
       directives.  */
    if ((code == OMP_TARGET_EXIT_DATA || code == OMP_TARGET_UPDATE)
        && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_POINTER
            || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_TO_PSET))
      remove = true;

Note that I'm not proposing to also do that for OpenACC at this point; I have one doubt about that I need to look into first, but don't have time for right now.
Comment 1 Thomas Schwinge 2019-12-13 11:53:09 UTC
Relatedly, since r228777 "Merge from gomp-4_1-branch to trunk" ("which brings in most of the OpenMP 4.5 support for C and C++"), the following optimization is done:

    case OMP_TARGET_ENTER_DATA:
    case OMP_TARGET_EXIT_DATA:
    case OACC_ENTER_DATA:
    case OACC_EXIT_DATA:
    case OACC_HOST_DATA:
      if (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FIRSTPRIVATE_POINTER
          || (OMP_CLAUSE_MAP_KIND (c)
              == GOMP_MAP_FIRSTPRIVATE_REFERENCE))
        /* For target {,enter ,exit }data only the array slice is
           mapped, but not the pointer to it.  */
        remove = true;
      break;

Why is this not applicable for OpenACC '!$acc update'/OpenMP '!$omp target update', too?  (This is now also relevant for C/C++.)
Comment 2 Thomas Schwinge 2019-12-13 12:01:19 UTC
(In reply to myself from comment #1)
>       if (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FIRSTPRIVATE_POINTER
>           || (OMP_CLAUSE_MAP_KIND (c)
>               == GOMP_MAP_FIRSTPRIVATE_REFERENCE))
>         /* For target {,enter ,exit }data only the array slice is
>            mapped, but not the pointer to it.  */
>         remove = true;

> Why is this not applicable for OpenACC '!$acc update'/OpenMP '!$omp target
> update', too?  (This is now also relevant for C/C++.)

I see I asked the same question before, and in <http://mid.mail-archive.com/574F1C6C.5020704@codesourcery.com> Cesar answered: "I suppose they can be added here, but lower_omp_target already ignores GOMP_MAP_FIRSTPRIVATE_POINTER for non-offloaded and data_region regions."
Comment 3 Thomas Schwinge 2019-12-13 12:13:47 UTC
(In reply to myself from comment #1)
>         /* For target {,enter ,exit }data only the array slice is
>            mapped, but not the pointer to it.  */

Some handling for OpenACC 'data' got added in r236678 "use firstprivate pointers for subarrays in c and c++", <http://mid.mail-archive.com/573244BE.5010708@codesourcery.com>, but is somewhat spread out, and not easy to follow:

    case OACC_DATA:
      if (TREE_CODE (TREE_TYPE (decl)) != ARRAY_TYPE)
        break;
      /* FALLTHRU */
    case OMP_TARGET_DATA:
    case [...]:
      if (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FIRSTPRIVATE_POINTER
          || (OMP_CLAUSE_MAP_KIND (c)
              == GOMP_MAP_FIRSTPRIVATE_REFERENCE))
        /* For target {,enter ,exit }data only the array slice is
           mapped, but not the pointer to it.  */
        remove = true;
[...]
    if (code == OACC_DATA
        && OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
        && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FIRSTPRIVATE_POINTER
            || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FIRSTPRIVATE_REFERENCE))
      remove = true;
    if (remove)
      *list_p = OMP_CLAUSE_CHAIN (c);

..., and not sure if this also relates to the following:

'omp_notice_variable':

    if (octx->region_type == ORT_ACC_DATA
    	  && (n2->value & GOVD_MAP_0LEN_ARRAY))
    	nflags |= GOVD_MAP_0LEN_ARRAY;
    goto found_outer;

'gimplify_scan_omp_clauses':

    if (code == OACC_DATA
        && OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
        && OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FIRSTPRIVATE_POINTER)
      flags |= GOVD_MAP_0LEN_ARRAY;

(Maybe the idea there is to preserve 'GOMP_MAP_FIRSTPRIVATE_POINTER', 'GOMP_MAP_FIRSTPRIVATE_REFERENCE' until some other processing ('GOVD_MAP_0LEN_ARRAY') has been done, and only then remove them, right at the end.  But that wouldn't be quite obvious.)
Comment 4 Thomas Schwinge 2019-12-21 19:16:13 UTC
The recent r279626 "OpenACC 2.6 deep copy: middle-end parts" contains changes related to this (which caused PR93026).

Would've been good to first sort out the desired behavior (this PR92929) before introducing new functionality (OpenACC 2.6 manual deep copy) that supposedly depends on these optimizations?

In particular, this needs careful attention, because we have to stay ABI compatible with existing GCC releases: libgomp needs to do the right things for both the "old" and the "new" way.  (I'm not claiming that it isn't -- just that it needs careful attention.)
Comment 5 Thomas Schwinge 2019-12-21 21:33:07 UTC
Author: tschwinge
Date: Sat Dec 21 21:32:36 2019
New Revision: 279700

URL: https://gcc.gnu.org/viewcvs?rev=279700&root=gcc&view=rev
Log:
[PR93026, PR92929] Adjust 'gfortran.dg/goacc/finalize-1.f' for r279626 changes

	gcc/testsuite/
	PR fortran/93026
	PR middle-end/92929
	* gfortran.dg/goacc/finalize-1.f: Adjust.

Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/goacc/finalize-1.f
Comment 6 jules 2020-01-02 16:55:39 UTC
Apologies for breakage. This part of the patch was originally from the og9 patch supporting Fortran polymorphic class pointers posted at https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00752.html. The rationale was as follows:

[...] OpenACC "enter data" and "exit data" now have GOMP_MAP_POINTER and
GOMP_MAP_PSET mappings removed during gimplification. In some
circumstances, passing an array to a function/subroutine and then doing
an "enter data" on it could leave dangling references to the function's
stack, although the actual array data is defined outside the function.
In any case, the pointer/pointer-set mappings don't seem to be
necessary for OpenACC "enter data".

I observed the described problem with a large test program, and unfortunately did not manage to come up with a minimised test case at the time. Someone more familiar with Fortran might be able to do so more easily than me!

In terms of backwards compatibility, we can't do anything about the "parasitic binding" to a function's expired stack frame as generated by an older version of the compiler in this case, I don't think. That's most likely going to manifest as an intermittent crash.
Comment 7 jules 2020-06-08 13:08:40 UTC
Test case & further discussion in:

https://gcc.gnu.org/pipermail/gcc-patches/2020-June/547424.html
Comment 8 Thomas Schwinge 2020-07-06 16:19:36 UTC
In addition to r277631 "Fortran/OpenMP] Don't create "alloc:" for 'target exit data'", as shown in the PR94635 commit af557050fd011a03d21dc26b31959033061a0443 "[OpenMP] Fix 'omp exit data' for Fortran arrays (PR 94635)", and commit 9f0f7da9aa98eec28b4e5e34ade0aa0028df161d "[OpenMP] Fix 'omp exit data' for Fortran arrays (PR 94635)", this is not only an optimization but also a correctness issue.

The OpenMP cases now presumably have been addressed, but OpenACC not yet?
Comment 9 Thomas Schwinge 2020-07-06 16:37:39 UTC
(In reply to Thomas Schwinge from comment #0)
> As of Tobias' recent r277631 "Fortran/OpenMP] Don't create "alloc:" for
> 'target exit data'",
> <http://mid.mail-archive.com/7a5f39e8-a33b-048a-f9c1-
> 1355b941771e@codesourcery.com>, this is done for Fortran OpenMP '!$omp
> target exit data'/'!$omp target update', but not for OpenACC '!$acc exit
> data'/'!$acc update'; see 'gcc/gimplify.c:gimplify_scan_omp_clauses':
> 
>     /* For Fortran, not only the pointer to the data is mapped but also
>        the address of the pointer, the array descriptor etc.; for
>        'exit data' - and in particular for 'delete:' - having an 'alloc:'
>        does not make sense.  Likewise, for 'update' only transferring the
>        data itself is needed as the rest has been handled in previous
>        directives.  */
>     if ((code == OMP_TARGET_EXIT_DATA || code == OMP_TARGET_UPDATE)
>         && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_POINTER
>             || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_TO_PSET))
>       remove = true;

Is it actually correct to remove 'GOMP_MAP_TO_PSET' for 'OMP_TARGET_UPDATE'?

Untested -- I'm thinking of the following Fortran scenario:

    arrays a1, a2
    pointer p

    [target] enter data(a1, a2)

    p => a1
    [target] enter data(p)
    ! creates persistent, visible device copy of 'p', initially pointing to 'a1'

    p => a2
    [target] update to(p)
    ! do we now still have 'p => a1' on the device, or 'p => a2'?

What is this '[target] update' to do?  Just update the data pointed to, and also/or only update the array descriptor on the device?  If the latter, then didn't get this broken by r277631 (cited above)?  Again: unverified.

But (also unverified), would 'libgomp/target.c:gomp_update' actually do the right thing for 'GOMP_MAP_TO_PSET', given the current 'GOMP_MAP_COPY_TO_P' definition?

Am I confused, or the code?
Comment 10 Thomas Schwinge 2020-07-14 12:51:12 UTC
(In reply to Thomas Schwinge from comment #4)
> The recent r279626 "OpenACC 2.6 deep copy: middle-end parts" contains
> changes related to this

... some of which have now gotten reverted in recent commit g:b20097c65d2e74b1901fba1c55c77f0407e542d2 "openacc: Don't strip TO_PSET/POINTER for enter/exit data".