libgomp/openacc.f90 – clean-up public/private attributes

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?
(If not, I will commit it in the next days.)

Tobias

PS: I found the two missing symbols by looking at the 'interface <name>' 
lines; 'module openacc' has only those + the version symbol.

PPS: I have filled PR 92913 as gfortran feature request to compare the 
'interface' block's procedure declarations with the actual procedure 
declarations outside of the modules; as, unfortunately, no 
argument-mismatch check exists, currently.

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