openmp: ignore nowait if async execution is unsupported [PR93481]

Hi Jakub,

On 10.02.20 08:49, Harwath, Frederik wrote:

>> There has been even in some PR a suggestion that instead of failing
>> in nvptx async_run we should just ignore the nowait clause if the plugin
>> doesn't implement it properly.
> 
> This must be https://gcc.gnu.org/PR93481.

The attached patch implements the behavior that has been suggested in the PR.
It makes GOMP_OFFLOAD_async_run optional, removes the stub which produces
the error described in the PR from the nvptx plugin, and changes the nowait-handling
to ignore the clause if GOMP_OFFLOAD_async_run is not available for the executing
device's plugin. I have tested the patch by running the full libgomp testsuite with
nvptx-none offloading on x86_64-linux-gnu. I have observed no regressions.

Ok to push the commit to master?

For the record: Someone should implement GOMP_OFFLOAD_async_run properly
in the nvtpx plugin.

Best regards,
Frederik

On Thu, Feb 13, 2020 at 09:04:36AM +0100, Harwath, Frederik wrote:
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -2022,6 +2022,16 @@ GOMP_target (int device, void (*fn) (void *), const void *unused,
>    gomp_unmap_vars (tgt_vars, true);
>  }
>  
> +static unsigned int

Add inline?

> +clear_unsupported_flags (struct gomp_device_descr *devicep, unsigned int flags)
> +{
> +  /* If we cannot run asynchronously, simply ignore nowait.  */
> +  if (devicep != NULL && devicep->async_run_func == NULL)
> +    flags &= ~GOMP_TARGET_FLAG_NOWAIT;
> +
> +  return flags;
> +}
> +
>  /* Like GOMP_target, but KINDS is 16-bit, UNUSED is no longer present,
>     and several arguments have been added:
>     FLAGS is a bitmask, see GOMP_TARGET_FLAG_* in gomp-constants.h.
> @@ -2054,6 +2064,8 @@ GOMP_target_ext (int device, void (*fn) (void *), size_t mapnum,
>    size_t tgt_align = 0, tgt_size = 0;
>    bool fpc_done = false;
>  
> +  flags = clear_unsupported_flags (devicep, flags);
> +
>    if (flags & GOMP_TARGET_FLAG_NOWAIT)
>      {
>        struct gomp_thread *thr = gomp_thread ();

LGTM.

> @@ -2257,6 +2269,8 @@ GOMP_target_update_ext (int device, size_t mapnum, void **hostaddrs,
>  {
>    struct gomp_device_descr *devicep = resolve_device (device);
>  
> +  flags = clear_unsupported_flags (devicep, flags);
> +
>    /* If there are depend clauses, but nowait is not present,
>       block the parent task until the dependencies are resolved
>       and then just continue with the rest of the function as if it
> @@ -2398,6 +2412,8 @@ GOMP_target_enter_exit_data (int device, size_t mapnum, void **hostaddrs,
>  {
>    struct gomp_device_descr *devicep = resolve_device (device);
>  
> +  flags = clear_unsupported_flags (devicep, flags);
> +
>    /* If there are depend clauses, but nowait is not present,
>       block the parent task until the dependencies are resolved
>       and then just continue with the rest of the function as if it

I don't see why you need to do the above two.  GOMP_TARGET_TASK_DATA
is done on the host side, async_run callback isn't called in that case
and while we create a task, all we do is wait for the (host) dependencies
in there and then perform the data transfer we need.
I think it is perfectly fine to ignore nowait on target but honor it
on target update or target {enter,exit} data.

> @@ -2524,6 +2540,7 @@ gomp_target_task_fn (void *data)
>  	}
>        ttask->state = GOMP_TARGET_TASK_READY_TO_RUN;
>  
> +      assert (devicep->async_run_func);
>        devicep->async_run_func (devicep->target_id, fn_addr, actual_arguments,
>  			       ttask->args, (void *) ttask);
>        return true;
> @@ -3040,7 +3057,7 @@ gomp_load_plugin_for_device (struct gomp_device_descr *device,
>    if (device->capabilities & GOMP_OFFLOAD_CAP_OPENMP_400)
>      {
>        DLSYM (run);
> -      DLSYM (async_run);
> +      DLSYM_OPT (async_run, async_run);
>        DLSYM_OPT (can_run, can_run);
>        DLSYM (dev2dev);
>      }
> diff --git a/libgomp/testsuite/libgomp.c/target-33.c b/libgomp/testsuite/libgomp.c/target-33.c
> index 15d2d7e38ab..1bed4b6bc67 100644
> --- a/libgomp/testsuite/libgomp.c/target-33.c
> +++ b/libgomp/testsuite/libgomp.c/target-33.c
> @@ -1,6 +1,3 @@
> -/* { dg-xfail-run-if "GOMP_OFFLOAD_async_run not implemented" { offload_target_nvptx } }
> -   Cf. https://gcc.gnu.org/PR81688.  */
> -
>  extern void abort (void);
>  
>  int
> diff --git a/libgomp/testsuite/libgomp.c/target-34.c b/libgomp/testsuite/libgomp.c/target-34.c
> index 5a3596424d8..66d9f54202b 100644
> --- a/libgomp/testsuite/libgomp.c/target-34.c
> +++ b/libgomp/testsuite/libgomp.c/target-34.c
> @@ -1,6 +1,3 @@
> -/* { dg-xfail-run-if "GOMP_OFFLOAD_async_run not implemented" { offload_target_nvptx } }
> -   Cf. https://gcc.gnu.org/PR81688.  */
> -
>  extern void abort (void);
>  
>  int
> -- 
> 2.17.1
> 
> 

Otherwise LGTM.

	Jakub

Hi Jakub,

On 13.02.20 09:30, Jakub Jelinek wrote:
> On Thu, Feb 13, 2020 at 09:04:36AM +0100, Harwath, Frederik wrote:
>> --- a/libgomp/target.c
>> +++ b/libgomp/target.c
>> @@ -2022,6 +2022,16 @@ GOMP_target (int device, void (*fn) (void *), const void *unused,
>>    gomp_unmap_vars (tgt_vars, true);
>>  }
>>  
>> +static unsigned int
> 
> Add inline?
> 

Added.

>> @@ -2257,6 +2269,8 @@ GOMP_target_update_ext (int device, size_t mapnum, void **hostaddrs,
>>  {
>>    struct gomp_device_descr *devicep = resolve_device (device);
>>  
>> +  flags = clear_unsupported_flags (devicep, flags);

>> @@ -2398,6 +2412,8 @@ GOMP_target_enter_exit_data (int device, size_t mapnum, void **hostaddrs,
>>  {
>>    struct gomp_device_descr *devicep = resolve_device (device);
>>  
>> +  flags = clear_unsupported_flags (devicep, flags);

> I don't see why you need to do the above two.  GOMP_TARGET_TASK_DATA
> is done on the host side, async_run callback isn't called in that case
> and while we create a task, all we do is wait for the (host) dependencies
> in there and then perform the data transfer we need.
> I think it is perfectly fine to ignore nowait on target but honor it
> on target update or target {enter,exit} data.

I see. Removed.


> Otherwise LGTM.

Thanks for the review! I have committed the patch with those changes. I forgot to include the ChangeLog
entry which I had to add in a separate commit. Sorry for that! It seems that I have to adapt my workflow -
perhaps some pre-push hook ;-).

Best regards,
Frederik