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
