Re: Fwd: [RFC][gomp4] Offloading patches (2/3): Add tables generation

[Thomas Schwinge <thomas@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 3021 bytes --]

Hi!

On Sat, 8 Mar 2014 18:50:15 +0400, Ilya Verbin <iverbin@gmail.com> wrote:
> --- a/libgomp/libgomp.map
> +++ b/libgomp/libgomp.map
> @@ -208,6 +208,7 @@ GOMP_3.0 {
>  
>  GOMP_4.0 {
>    global:
> +	GOMP_offload_register;
>  	GOMP_barrier_cancel;
>  	GOMP_cancel;
>  	GOMP_cancellation_point;

Now that the GOMP_4.0 symbol version is being used in GCC trunk, and will
be in the GCC 4.9 release, can we still add new symbols to it here?
(Jakub?)

> --- a/libgomp/plugin-host.c
> +++ b/libgomp/plugin-host.c

> +const int TARGET_TYPE_HOST = 0;

We'll have to see whether this (that is, libgomp/target.c:enum
target_type) should live in a shared header file, but OK for the moment.

> +void
> +device_run (void *fn_ptr, void *vars)
> +{
> +#ifdef DEBUG
> +  printf ("libgomp plugin: %s:%s (%p, %p)\n", __FILE__, __FUNCTION__, fn_ptr,
> +	  vars);
> +#endif
> +
> +  void (*fn)(void *) = (void (*)(void *)) fn_ptr;
> +
> +  fn (vars);
> +}

Why not make fn_ptr a proper function pointer?  Ah, because of
GOMP_target passing (void *) tgt_fn->tgt->tgt_start for the
!TARGET_TYPE_HOST case...

Would it make sense to have device_run return a value to make it able to
indicate to libgomp that the function cannot be run on the device (for
whatever reason), and libgomp should use host-fallback execution?
(Probably that needs more thought and discussion, OK to defer.)

> --- a/libgomp/target.c
> +++ b/libgomp/target.c

> +enum target_type {
> +  TARGET_TYPE_HOST,
> +  TARGET_TYPE_INTEL_MIC
> +};

(As discussed above, but OK to defer.)

> @@ -120,15 +140,26 @@ struct gomp_device_descr
>       TARGET construct.  */
>    int id;
>  
> +  /* This is the TYPE of device.  */
> +  int type;

Use enum target_type instead of int?

> +/* This function should be called from every offload image.  It gets the
> +   descriptor of the host func and var tables HOST_TABLE, TYPE of the target,
> +   and TARGET_DATA needed by target plugin (target tables, etc.)  */
> +void
> +GOMP_offload_register (void *host_table, int type, void *target_data)
> +{
> +  offload_images = realloc (offload_images,
> +			    (num_offload_images + 1)
> +			    * sizeof (struct offload_image_descr));
> +
> +  if (offload_images == NULL)
> +    return;

Fail silently, or use gomp_realloc to fail loudly?

> @@ -701,16 +836,25 @@ gomp_find_available_plugins (void)

> - out:
> +out:

Emacs wants the space to be there, so I assume that's the coding standard
to use.  ;-)

>    if (dir)
>      closedir (dir);
> +  free (offload_images);

I suggest to set offload_images = NULL, for clarity.

> +  num_offload_images = 0;
>  }

We may need to revisit this later: currently it's not possible to
register additional plugins after libgomp has initialized
(gomp_target_init, gomp_find_available_plugins just executed once), but
should that ever be made possible, we'd need to preserve offload_images.


OK to commit, thanks!


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --]