On Tue, Oct 08, 2019 at 01:11:53AM +0200, Tobias Burnus wrote:
>       gcc/fortran/
>       * f95-lang.c (LANG_HOOKS_OMP_IS_ALLOCATABLE_OR_PTR): Re-define to
>       gfc_omp_is_allocatable_or_ptr.
>       * trans-decl.c (create_function_arglist): Set GFC_DECL_OPTIONAL_ARGUMENT
>       only if not passed by value.
>       * trans-openmp.c (gfc_omp_is_allocatable_or_ptr): New.
>       (gfc_trans_omp_clauses): Actually pass use_device_addr on to the middle
>       end; for MAP, handle (present) optional arguments; for target update,
>       handle allocatable/pointer scalars.
>       * trans.h (gfc_omp_is_allocatable_or_ptr): Declare.
> 
>       gcc/
>       * langhooks-def.h (LANG_HOOKS_OMP_IS_ALLOCATABLE_OR_PTR): Define.
>       (LANG_HOOKS_DECLS): Add it.
>       * langhooks.h (lang_hooks_for_decls): Add omp_is_allocatable_or_ptr;
>       update comment for omp_is_optional_argument.
>       * omp-general.c (omp_is_allocatable_or_ptr): New.
>       * omp-general.h (omp_is_allocatable_or_ptr): Declare.
>       * omp-low.c (scan_sharing_clauses, lower_omp_target): Handle
>       Fortran's optional arguments and allocatable/pointer scalars
>       with use_device_addr.

This looks reasonable, with a small nit.

>       libgomp/
>       * testsuite/libgomp.fortran/use_device_addr-1.f90: New.
>       * testsuite/libgomp.fortran/use_device_addr-2.f90: New.

I'm worried about the tests though.

> @@ -11678,7 +11680,18 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, 
> omp_context *ctx)
>                 }
>               else
>                 {
> -                 var = build_fold_addr_expr (var);
> +                 // While MAP is handled explicitly by the FE,
> +                 // for 'target update', only the identified is passed.

omp-low.c (like most of gcc/*.c) uses /* ... */ comments almost everywhere,
can you please just use the same here?
Otherwise the non-test part LGTM.

What worries me about the test is that the officially only portable way
to use it in a target region is is_device_ptr.  I believe the intention was
to allow the implementation to transform the pointers from e.g.
use_device_ptr to whatever the implementation wants, where it could be e.g.
a structure containing pointer and something or whatever and is_device_ptr
actually finishing it up, even when in our implementation is_device_ptr is
basically just copying the pointer bits from host to device (==
firstprivate).
Yes, I know there is the restriction that the is_device_ptr list item must
be a dummy variable without VALUE/ALLOCATABLE/POINTER.  VALUE itself
wouldn't be a big deal, we could call just by reference instead of value,
but allocatable/pointer I bet is a problem.  So, if there is no portable
way in Fortran to pass c_loc result as a dummy argument to some subprogram
where the dummy argument is not allocatable/pointer, and the caller doesn't
access the actual data in any way, I'm afraid we need to do what you are
doing.  But then the test should start with a comment that it is not
portable and assumes that is_device_ptr doesn't need to transform the
use_device_ptr addresses in any way.  Or another option would be to use C
code for the actual target region, c_loc is for C pointers and if you pass
to C code the c_loc as a pointer and pass in the array size/whatever else it
needs to know, it can then implement it portably with is_device_ptr clause
on the C pointer.

        Jakub

Reply via email to