Hi Tobias!
On 2023-06-12T18:44:23+0200, Tobias Burnus <[email protected]> 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
