Re: [Patch][v5] OpenMP: Move omp requires checks to libgomp

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Jul 01, 2022 at 11:08:16PM +0200, Tobias Burnus wrote:
>    gomp_mutex_lock (&register_lock);
>  
> +  if (omp_requires_mask && omp_requires_mask != omp_req)

I'd use if (omp_req && omp_requires_mask && omp_requires_mask != omp_req)
e.g. for the case of mixing GCC <= 12 compiled code with GCC 13,
treat omp_req 0 as "don't know" while GOMP_REQUIRES_TARGET_USED
as "known and no requires uni*/rev* specified".

> +    {
> +      char buf1[sizeof ("unified_address, unified_shared_memory, "
> +			"reverse_offload")];
> +      char buf2[sizeof ("unified_address, unified_shared_memory, "
> +			"reverse_offload")];
> +      gomp_requires_to_name (buf2, sizeof (buf2),
> +			     omp_req != GOMP_REQUIRES_TARGET_USED
> +			     ? omp_req : omp_requires_mask);
> +      if (omp_req != GOMP_REQUIRES_TARGET_USED
> +	  && omp_requires_mask != GOMP_REQUIRES_TARGET_USED)
> +	{
> +	  gomp_requires_to_name (buf1, sizeof (buf1), omp_requires_mask);
> +	  gomp_fatal ("OpenMP 'requires' directive with non-identical clauses "
> +		      "in multiple compilation units: '%s' vs. '%s'",
> +		      buf1, buf2);
> +	}
> +      else
> +	gomp_fatal ("OpenMP 'requires' directive with '%s' specified only in "
> +		    "some compilation units", buf2);
> +    }
> +  omp_requires_mask = omp_req;
> +
>    /* Load image to all initialized devices.  */
>    for (i = 0; i < num_devices; i++)
>      {
> @@ -4125,8 +4173,30 @@ gomp_target_init (void)
>  
>  	if (gomp_load_plugin_for_device (&current_device, plugin_name))
>  	  {
> -	    new_num_devs = current_device.get_num_devices_func ();
> -	    if (new_num_devs >= 1)
> +	    int omp_req = omp_requires_mask & ~GOMP_REQUIRES_TARGET_USED;
> +	    new_num_devs = current_device.get_num_devices_func (omp_req);
> +	    if (new_num_devs < 0)

Can this be if (gomp_debug && new_num_devs < 0) - i.e. be verbose only
when the user asks for it?

> +	      {
> +		bool found = false;
> +		int type = current_device.get_type_func ();
> +		for (int img = 0; img < num_offload_images; img++)
> +		  if (type == offload_images[img].type)
> +		    found = true;
> +		if (found)
> +		  {
> +		    char buf[sizeof ("unified_address, unified_shared_memory, "
> +				     "reverse_offload")];
> +		    gomp_requires_to_name (buf, sizeof (buf), omp_req);
> +		    char *name = (char *) malloc (cur_len + 1);
> +		    memcpy (name, cur, cur_len);
> +		    name[cur_len] = '\0';
> +		    GOMP_PLUGIN_error ("note: %s devices present but 'omp "
> +				       "requires %s' cannot be fulfilled",
> +				       name, buf);
> +		    free (name);
> +		  }
> +	      }
> +	    else if (new_num_devs >= 1)
>  	      {
>  		/* Augment DEVICES and NUM_DEVICES.  */
>  

Otherwise LGTM.

	Jakub