Re: [committed] OpenMP: Cleanups related to the 'present' modifier

[Thomas Schwinge <thomas@codesourcery.com>]

Hi Tobias!

On 2023-06-12T18:44:23+0200, Tobias Burnus <tobias@codesourcery.com> wrote:
> Cleanup follow up to
>    r14-1579-g4ede915d5dde93 "openmp: Add support for the 'present' modifier"
> committed 6 days ago.
>
> Namely:
> * Replace for the program → libgomp ABI GOMP_MAP_PRESENT_[ALLOC,TO,FROM,TOFROM]
>    by the preexisting GOMP_MAP_FORCE_PRESENT but keep the other enum values
>    (and use them until gimplifcation).
>
> * Improve wording if a non-existing/unsupported map-type modifier was used
>    by not referring to 'omp target' as it could be also target (enter/exit) data.
>    + Add a testcase for enter/exit data + data.
>
> * Unify + improve wording shown for 'present' when not present on the device.
>
> * Extend in the testcases to check that data actually gets copied with
>    'target update' and 'map when the 'present' modifier is present.
>
> Committed as Rev. r14-1736-g38944ec2a6fa10

>     OpenMP: Cleanups related to the 'present' modifier
>
>     Reduce number of enum values passed to libgomp as
>     GOMP_MAP_PRESENT_{TO,TOFROM,FROM,ALLOC} have the same semantic as
>     GOMP_MAP_FORCE_PRESENT (i.e. abort if not present, otherwise ignore);
>     that's different to GOMP_MAP_ALWAYS_PRESENT_{TO,TOFROM,FROM} which also
>     abort if not present but copy data when present. This is is a follow-up to
>     the commit r14-1579-g4ede915d5dde93 done 6 days ago.

Great, that matches how I thought this should be done (re our 2023-06-07
GCC IRC discussion).

>     Additionally, the commit [...]
>     extends testcases a tiny bit.

>     gcc/testsuite/ChangeLog:

>             * gfortran.dg/gomp/target-update-1.f90: Likewise.

That one addressed fixed <https://gcc.gnu.org/110178>
"gfortran.dg/gomp/target-update-1.f90 fails after r14-1579-g4ede915d5dde93".

> --- a/include/gomp-constants.h
> +++ b/include/gomp-constants.h

|  #define GOMP_MAP_FLAG_PRESENT                (GOMP_MAP_FLAG_SPECIAL_5 \
|                                        | GOMP_MAP_FLAG_SPECIAL_0)

Couldn't/shouldn't we now get rid of this 'GOMP_MAP_FLAG_PRESENT'...

|  #define GOMP_MAP_FLAG_ALWAYS_PRESENT (GOMP_MAP_FLAG_SPECIAL_2 \
|                                        | GOMP_MAP_FLAG_PRESENT)

..., as it is only used in 'GOMP_MAP_FLAG_ALWAYS_PRESENT' here...

> @@ -136,14 +136,6 @@ enum gomp_map_kind
>         device.  */
>      GOMP_MAP_ALWAYS_TOFROM =         (GOMP_MAP_FLAG_SPECIAL_2
>                                        | GOMP_MAP_TOFROM),
> -    /* Must already be present.  */
> -    GOMP_MAP_PRESENT_ALLOC =         (GOMP_MAP_FLAG_PRESENT | GOMP_MAP_ALLOC),
> -    /* Must already be present, copy to device.  */
> -    GOMP_MAP_PRESENT_TO =            (GOMP_MAP_FLAG_PRESENT | GOMP_MAP_TO),
> -    /* Must already be present, copy from device.  */
> -    GOMP_MAP_PRESENT_FROM =          (GOMP_MAP_FLAG_PRESENT | GOMP_MAP_FROM),
> -    /* Must already be present, copy to and from device.  */
> -    GOMP_MAP_PRESENT_TOFROM =                (GOMP_MAP_FLAG_PRESENT | GOMP_MAP_TOFROM),
>      /* Must already be present, unconditionally copy to device.  */
>      GOMP_MAP_ALWAYS_PRESENT_TO =     (GOMP_MAP_FLAG_ALWAYS_PRESENT
>                                        | GOMP_MAP_TO),
> @@ -205,7 +197,13 @@ enum gomp_map_kind
>      /* An attach or detach operation.  Rewritten to the appropriate type during
>         gimplification, depending on directive (i.e. "enter data" or
>         parallel/kernels region vs. "exit data").  */
> -    GOMP_MAP_ATTACH_DETACH =         (GOMP_MAP_LAST | 3)
> +    GOMP_MAP_ATTACH_DETACH =         (GOMP_MAP_LAST | 3),
> +    /* Must already be present - all of following map to GOMP_MAP_FORCE_PRESENT
> +       as no data transfer is needed.  */
> +    GOMP_MAP_PRESENT_ALLOC =         (GOMP_MAP_LAST | 4),
> +    GOMP_MAP_PRESENT_TO =            (GOMP_MAP_LAST | 5),
> +    GOMP_MAP_PRESENT_FROM =          (GOMP_MAP_LAST | 6),
> +    GOMP_MAP_PRESENT_TOFROM =                (GOMP_MAP_LAST | 7)
>    };
>
>  #define GOMP_MAP_COPY_TO_P(X) \
> @@ -243,7 +241,8 @@ enum gomp_map_kind
>    (((X) & GOMP_MAP_FLAG_SPECIAL_BITS) == GOMP_MAP_FLAG_FORCE)
>
>  #define GOMP_MAP_PRESENT_P(X) \
> -  (((X) & GOMP_MAP_FLAG_PRESENT) == GOMP_MAP_FLAG_PRESENT)
> +  (((X) & GOMP_MAP_FLAG_PRESENT) == GOMP_MAP_FLAG_PRESENT \
> +   || (X) == GOMP_MAP_FORCE_PRESENT)

..., and this 'GOMP_MAP_PRESENT_P' should look for
'GOMP_MAP_FLAG_ALWAYS_PRESENT' instead of 'GOMP_MAP_FLAG_PRESENT' (plus
'GOMP_MAP_FORCE_PRESENT')?

Instead of the current effective 'GOMP_MAP_FLAG_ALWAYS_PRESENT':

    GOMP_MAP_FLAG_SPECIAL_0
    | GOMP_MAP_FLAG_SPECIAL_2
    | GOMP_MAP_FLAG_SPECIAL_5

..., it could/should use a simpler flag combination?  (My idea is that
this later make usage of flag bits for other purposes easier -- but I've
not verified that in depth.)


Grüße
 Thomas
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955