Re: [PATCH 1/5] OpenACC 2.0 support for libgomp - OpenACC runtime, NVidia PTX/CUDA plugin (repost)

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Nov 11, 2014 at 01:53:23PM +0000, Julian Brown wrote:
> A few OpenMP tests fail with the new host_nonshm plugin (with failures
> of the form "libgomp: Trying to update [0x605820..0x605824) object that
> is not mapped"), probably because of middle-end bugs. I haven't
> investigated those in detail.

Depends how exactly your host_nonshm plugin works.  A few tests in the
testsuite use #pragma omp declare target variables, so if host_nonshm
plugin is something like I had on the gomp-4_0-branch initially as
hackish device 257, where code is run on the host, and map directives simply
malloc/free host memory and memcpy stuff around, then without extra work
the #pragma omp declare target variables indeed can't work.
You'd either need to support a strange partially shared memory model,
where #pragma omp declare target variables would be shared (you'd still
need to populate the mapping data structures with those vars and identity
map them), or not so conforming model where you'd map them on entering
the target regions if they aren't mapped yet (the thing is that then
if the variables are changed on the host in between the start of the program
and the target region, you'd use the changed values instead the values
they were originally assigned), or map them in some constructor (but, how
would you know if a host_nonshm plugin is going to be used in the future).

One can always use the intelmicemul plugin to test nonshared-memory stuff
without any HW (provided the host is x86_64/i686), so do we really need
host_nonshm plugin?

> --- a/libgomp/configure.ac
> +++ b/libgomp/configure.ac
> @@ -2,6 +2,8 @@
>  # aclocal -I ../config && autoconf && autoheader && automake
>  
>  AC_PREREQ(2.64)
> +#TODO: Update for OpenACC?  But then also have to update copyright notices in
> +#all source files...

Please drop this.

> @@ -1181,6 +1197,7 @@ initialize_env (void)
>        gomp_global_icv.thread_limit_var
>  	= thread_limit_var > INT_MAX ? UINT_MAX : thread_limit_var;
>      }
> +  parse_int ("GCC_ACC_NOTIFY", &goacc_notify_var, true);

I would have expected GACC_NOTIFY name instead (or GOACC_NOTIFY)
to match GOMP_SPINCOUNT and similar env vars.

> +  /* Initialize OpenACC-specific internal state.  */
> +  ACC_runtime_initialize ();

Is there any need for the capital letters in the function name?

> -static void
> +void
>  gomp_verror (const char *fmt, va_list list)
>  {
>    fputs ("\nlibgomp: ", stderr);
> @@ -54,13 +54,39 @@ gomp_error (const char *fmt, ...)
>  }
>  
>  void
> +gomp_vfatal (const char *fmt, va_list list)
> +{
> +  gomp_verror (fmt, list);
> +  exit (EXIT_FAILURE);
> +}

You should add noreturn attribute to gomp_vfatal prototype in the header.

> +
> +void
>  gomp_fatal (const char *fmt, ...)
>  {
>    va_list list;
>  
>    va_start (list, fmt);
> -  gomp_verror (fmt, list);
> +  gomp_vfatal (fmt, list);
>    va_end (list);
>  
> -  exit (EXIT_FAILURE);
> +  /* Unreachable.  */
> +  abort ();

And there is no need for the abort here.

> +extern int goacc_notify_var;
> +extern int goacc_device_num;
> +extern char* goacc_device_type;

See above.

> @@ -532,8 +538,12 @@ extern void *gomp_realloc (void *, size_t);
>  
>  /* error.c */
>  
> +extern void gomp_vnotify (const char *, va_list);
> +extern void gomp_notify (const char *msg, ...);
> +extern void gomp_verror (const char *, va_list);
>  extern void gomp_error (const char *, ...)
>  	__attribute__((format (printf, 1, 2)));
> +extern void gomp_vfatal (const char *, va_list);

See above.  Also, please add format attributes too for all the new
prototypes here.

>  extern void gomp_fatal (const char *, ...)
>  	__attribute__((noreturn, format (printf, 1, 2)));
>  

> +OACC_2.0 {
> +  global:
> +	acc_get_num_devices;
> +	acc_get_num_devices_h_;

Somebody recently suggested (for OpenMP) that we just should use
bind(C) in the Fortran module, it is too late for OpenMP, as we
have to keep the *_ entrypoints for compatibility anyway, but
for OpenACC and new OpenMP functions supposedly you could avoid
exporting all the *_ wrappers and use * directly.

> +PLUGIN_1.0 {

Perhaps GOMP_PLUGIN_1.0 instead?

> +  global:
> +	GOMP_PLUGIN_malloc;
> +	GOMP_PLUGIN_malloc_cleared;
> +	GOMP_PLUGIN_realloc;
> +	GOMP_PLUGIN_error;
> +	GOMP_PLUGIN_notify;
> +	GOMP_PLUGIN_fatal;
> +	GOMP_PLUGIN_mutex_init;
> +	GOMP_PLUGIN_mutex_destroy;
> +	GOMP_PLUGIN_mutex_lock;
> +	GOMP_PLUGIN_mutex_unlock;
> +	GOMP_PLUGIN_async_unmap_vars;
> +	GOMP_PLUGIN_acc_thread;
> +};
> +
> +# TODO.  See testsuite/lib/libgomp.exp:libgomp_init.
> +INTERNAL {
> +  global:
> +	initialize_env;
> +};

Ugh, I don't like that.  If it is a hack around dejagnu deficiency, then
perhaps dejagnu should be changed or gcc *.exp adjusted, if it is for
all programs, then there should be some way how to communicate passing
state from the host to the target plugin.

> +typedef struct ACC_dispatch_t

Can't you just use acc_dispatch_t ?
I'd prefer the capital prefixes just for functions called from
compiler generated code (like GOMP_* entrypoints; so GACC_*) and
perhaps if the standard mandates some other functions/structures to be
upper-case, or for the functions plugin calls from libgomp or libgomp
looks up in the plugins, but not elsewhere.

> +/* This is called when plugins have been initialized, and serves to call
> +   (indirectly) the target's device_init hook.  Calling multiple times without
> +   an intervening _acc_shutdown call is an error.  */
> +
> +static struct gomp_device_descr const *
> +_acc_init (acc_device_t d)

Why the underscore prefix?  Can't it clash with reserved namespaces?

> +static void
> +dump_var (char *s, size_t idx, void *hostaddr, size_t size, unsigned char kind)
> +{
> +  gomp_notify(" %2zi: %3s 0x%.2x -", idx, s, kind & 0xff);

Formatting, missing space before ( (many times).

> +  gomp_notify("- %d - %4d/0x%04x ", 1 << (kind >> 8), (int)size, (int)size);

And space after (int).

> +
> +  gomp_notify ("%s: mapnum=%zd, hostaddrs=%p, sizes=%p, kinds=%p\n",
> +	       __FUNCTION__, mapnum, hostaddrs, sizes, kinds);

Isn't such debugging too costly?  Perhaps either enable it only in
debugging builds, or at least guard with (perhaps in a gomp_notify macro)
with
  if (__builtin_expect (goacc_notify_var, 0))
    (gomp_notify) (__VA_ARGS__)
?  I'd think doing it in debugging builds only should be sufficient.
> +
> +  void ACC_async_copy(int) __GOACC_NOTHROW;
> +  void ACC_async_kern(int) __GOACC_NOTHROW;

Formatting.  Please check the missing spaces before ( everywhere.

> +} cuErrorList[] = {

This is GCC code, is there any need for CamelCase?
> +static char *
> +cuErrorMsg (CUresult r)

Ditto.

> +  { _XSTR(cuCtxCreate) },

Missing space before (.
> +  { _XSTR(cuCtxDestroy) },
...

	Jakub