Message ID | 2ac801b8-4dc4-62bc-10d8-e222af3bc304@codesourcery.com |
---|---|
State | New |
Headers | show |
Series | libgomp/openacc.f90 – clean-up public/private attributes | expand |
Hi Tobias! On 2019-12-11T13:47:51+0100, Tobias Burnus <tobias@codesourcery.com> wrote: > This patch cleans up the public/private handling in libgomp/openacc.f90: > > * module openacc_kinds marked symbols explicitly as public and private > (but default is public). Make this explicit by a 'PUBLIC' and remove the > (redundant) explicit 'public :: <symb>' lines. > > * 'module openacc' had a bunch of 'public :: ' lines but the default was > already 'public'. Changed this to 'private' and marked the > use-associated 'openacc_kinds' symbols as 'public' and added 'public' > statements for the two missing items. (Net effect: this will hide all > openacc_internal symbols.) > > I think this patch is rather obvious. Nonetheless: are the comments? Sounds good, don't have any comments on the Fortran details ;-) -- but I'll point out there also exists 'libgomp/config/accel/openacc.f90', which probably needs similar treatment? > (If not, I will commit it in the next days.) (Got committed in r279337.) > PS: I found the two missing symbols by looking at the 'interface <name>' > lines; 'module openacc' has only those + the version symbol. Ouch. Ah, no: you said "the default was already 'public'" -- so there's no need to backport that to gcc-9-branch, so that 'acc_copyout_finalize', 'acc_delete_finalize' will be available for OpenACC Fortran users. (These functions are, what would you expect, no covered by any test case in 'libgomp.oacc-fortran/'...) A few comments anyway: > --- a/libgomp/openacc.f90 > +++ b/libgomp/openacc.f90 > @@ -31,13 +31,12 @@ module openacc_kinds > use iso_fortran_env, only: int32 > implicit none > > + public > private :: int32 > - public :: acc_device_kind > > - integer, parameter :: acc_device_kind = int32 > + ! When adding items, also update 'public' setting in 'module openmp' below. This isn't "'module openmp'". ;-) > > - public :: acc_device_none, acc_device_default, acc_device_host > - public :: acc_device_not_host, acc_device_nvidia (So this was missing 'acc_device_gcn' -- but 'public' was the default.) > + integer, parameter :: acc_device_kind = int32 > > ! Keep in sync with include/gomp-constants.h. > integer (acc_device_kind), parameter :: acc_device_none = 0 > @@ -48,16 +47,11 @@ module openacc_kinds > integer (acc_device_kind), parameter :: acc_device_nvidia = 5 > integer (acc_device_kind), parameter :: acc_device_gcn = 8 > > - public :: acc_handle_kind > - > integer, parameter :: acc_handle_kind = int32 > > - public :: acc_async_noval, acc_async_sync > - > ! Keep in sync with include/gomp-constants.h. > integer (acc_handle_kind), parameter :: acc_async_noval = -1 > integer (acc_handle_kind), parameter :: acc_async_sync = -2 > - > end module > > module openacc_internal > @@ -717,6 +711,13 @@ module openacc > use openacc_internal > implicit none > > + private > + ! From openacc_kinds > + public :: acc_device_kind, acc_handle_kind > + public :: acc_device_none, acc_device_default, acc_device_host > + public :: acc_device_not_host, acc_device_nvidia, acc_device_gcn > + public :: acc_async_noval, acc_async_sync > + Some vertical space before the "From openacc_kinds" comment, and before the 'acc_async_*' ones? > public :: openacc_version > > public :: acc_get_num_devices, acc_set_device_type, acc_get_device_type > @@ -730,6 +731,7 @@ module openacc > public :: acc_update_device, acc_update_self, acc_is_present > public :: acc_copyin_async, acc_create_async, acc_copyout_async > public :: acc_delete_async, acc_update_device_async, acc_update_self_async > + public :: acc_copyout_finalize, acc_delete_finalize Put these into the place where they really belong, after 'acc_copyout, and 'acc_delete', respectively? Grüße Thomas
Hi Thomas, updated version committed (r279456) – which adds 'acc_device_gcn' to openacc_lib.h – besides the other cleanup. On 12/16/19 9:41 PM, Thomas Schwinge wrote > I'll point out there also exists 'libgomp/config/accel/openacc.f90', I have now updated that file in the same way – and added a cross-ref comment to both libgomp/openacc.f90 and libgomp/openacc_lib.h. > Ouch. Ah, no: you said "the default was already 'public'" -- so > there's no need to backport that to gcc-9-branch Yes, before, it was just a stylistic problem. > so that 'acc_copyout_finalize', 'acc_delete_finalize' will be > available for OpenACC Fortran users. (These functions are, what would > you expect, no covered by any test case in 'libgomp.oacc-fortran/'...) They are also not documented in libgomp.texi – nor are some of the _async_ ones. They are at least tested in C/C++ [via libgomp.oacc-c-c++-common/lib-32.c (both) and libgomp.oacc-c-c++-common/pr92843-1.c (acc_copyout_finalize, only)]. > This isn't "'module openmp'". ;-) Is it not? ;-) > Some vertical space before the "From openacc_kinds" comment, and > before the 'acc_async_*' ones I added one before – but not after to make is visually belong to that block. >> public :: acc_update_device, acc_update_self, acc_is_present >> public :: acc_copyin_async, acc_create_async, acc_copyout_async >> public :: acc_delete_async, acc_update_device_async, acc_update_self_async >> + public :: acc_copyout_finalize, acc_delete_finalize > Put these into the place where they really belong, after 'acc_copyout, > and 'acc_delete', respectively? I left them there, for now. The <name>_async variants are also in one block and not sorted after their respective <name> variants. Thanks for the suggestions & hints! Cheers, Tobias
2019-12-11 Tobias Burnus <tobias@codesourcery.com> libgomp/ * openacc.f90 (module openacc_kinds): Use 'PUBLIC' to mark all symbols as public except for the 'use …, only' imported symbol, which is private. (module openacc): Default to 'PRIVATE' to exclude openacc_internal; mark all symbols from module openacc_kinds as PUBLIC; add missing PUBLIC attributes for acc_copyout_finalize and acc_delete_finalize. diff --git a/libgomp/openacc.f90 b/libgomp/openacc.f90 index 831a157e703..b37f1872d50 100644 --- a/libgomp/openacc.f90 +++ b/libgomp/openacc.f90 @@ -31,13 +31,12 @@ module openacc_kinds use iso_fortran_env, only: int32 implicit none + public private :: int32 - public :: acc_device_kind - integer, parameter :: acc_device_kind = int32 + ! When adding items, also update 'public' setting in 'module openmp' below. - public :: acc_device_none, acc_device_default, acc_device_host - public :: acc_device_not_host, acc_device_nvidia + integer, parameter :: acc_device_kind = int32 ! Keep in sync with include/gomp-constants.h. integer (acc_device_kind), parameter :: acc_device_none = 0 @@ -48,16 +47,11 @@ module openacc_kinds integer (acc_device_kind), parameter :: acc_device_nvidia = 5 integer (acc_device_kind), parameter :: acc_device_gcn = 8 - public :: acc_handle_kind - integer, parameter :: acc_handle_kind = int32 - public :: acc_async_noval, acc_async_sync - ! Keep in sync with include/gomp-constants.h. integer (acc_handle_kind), parameter :: acc_async_noval = -1 integer (acc_handle_kind), parameter :: acc_async_sync = -2 - end module module openacc_internal @@ -717,6 +711,13 @@ module openacc use openacc_internal implicit none + private + ! From openacc_kinds + public :: acc_device_kind, acc_handle_kind + public :: acc_device_none, acc_device_default, acc_device_host + public :: acc_device_not_host, acc_device_nvidia, acc_device_gcn + public :: acc_async_noval, acc_async_sync + public :: openacc_version public :: acc_get_num_devices, acc_set_device_type, acc_get_device_type @@ -730,6 +731,7 @@ module openacc public :: acc_update_device, acc_update_self, acc_is_present public :: acc_copyin_async, acc_create_async, acc_copyout_async public :: acc_delete_async, acc_update_device_async, acc_update_self_async + public :: acc_copyout_finalize, acc_delete_finalize integer, parameter :: openacc_version = 201306