Re: Openacc launch API

[Nathan Sidwell <nathan at acm dot org>]

On 09/17/15 05:36, Bernd Schmidt wrote:

> Fail how? Jakub has requested that it works but falls back to unaccelerated
> execution, can you confirm this is what you expect to happen with this patch?

Yes, that is the failure mode.

> > -  if (num_waits)
> > +  va_start (ap, kinds);
> > +  /* TODO: This will need amending when device_type is implemented.  */
>
> I'd expect that this will check whether the device type in the argument is
> either zero (or whatever indicates all devices) or matches the current device.
> Is that what you intend?

correct.

> > +  while (GOMP_LAUNCH_PACK (GOMP_LAUNCH_END, 0, 0)
> > +     != (tag = va_arg (ap, unsigned)))
>
> That's a somewhat non-idiomatic way to write this, with the constant first and
> not obviously a constant. I'd initialize a variable with the constant before the
> loop.

Hm, yeah, that is a little unpleasant.  The alternative is IIRC a mid-loop break.

(If only this was C++  then we could write:

  while (int tag = va_arg (ap, unsigned)) { ... }

relying on GOMP_LAUNCH_PACK (GOMP_LAUNCH_END, 0, 0) being zero.  Maybe the 
C-ified version of that would be clearer?)

> +      assert (!GOMP_LAUNCH_DEVICE (tag));
>
> Uh, that seems unfriendly, and not exactly forwards compatible. Can that fail a

I suppose.  I was thinking of this as an internal interface from the compiler, 
but I guess down the road some one  could try  running a device_type implemented 
binary from a future compiler with an old libgomp.

> > +  if (num_waits > 8)
> > +    gomp_fatal ("Too many waits for legacy interface");
>
> How did you arrive at this number?

The voice in my head.  I've only seen code that had up to 2 waits, so I figured 
8 was way plenty.

> > +GOACC_2.0,1 {
> > +  global:
> > +    GOACC_parallel_keyed;
> > +} GOACC_2.0;
>
> Did you mean to use a comma?

Probably.

> > +  if (dims[GOMP_DIM_GANG] != 1)
> > +    GOMP_PLUGIN_fatal ("non-unity num_gangs (%d) not supported",
> > +               dims[GOMP_DIM_GANG]);
> > +  if (dims[GOMP_DIM_WORKER] != 1)
> > +    GOMP_PLUGIN_fatal ("non-unity num_workers (%d) not supported",
> > +               dims[GOMP_DIM_WORKER]);
>
> I see that this is just moved here (which is good), but is this still a
> limitation? Or is that on trunk only?

Trunk only.  It'll go away shortly when more patches get merged.


> > +static void
> > +set_oacc_fn_attrib (tree fn, tree clauses, vec<tree> *args)
> > +tree
> > +get_oacc_fn_attrib (tree fn)
>
> These need function comments.

oops.

> > +  /* Must match GOMP_DIM ordering.  */
> > +  static const omp_clause_code ids[] =
> > +    {OMP_CLAUSE_NUM_GANGS, OMP_CLAUSE_NUM_WORKERS, OMP_CLAUSE_VECTOR_LENGTH};
>
> Formatting. No = at the end of a line, and whitespace around braces.

oh, I thought intialization placed the = where I had -- didn't  know that nor 
the space-brace rule.

> > @@ -9150,6 +9245,7 @@ expand_omp_target (struct omp_region *re
> >       }
> >
> >     gimple g;
> > +  bool tagging = false;
> >     /* The maximum number used by any start_ix, without varargs.  */
>
> That looks misindented, but may be an email client thing.

It does, doesn't it.  Appears to be email artifact or something.

> > +    else if (!tagging)
>
> Oh... so tagging controls two different methods for constructing argument lists,
> one for GOACC_parallel and the other for whatever OMP uses? That's a bit
> unfortunate, I'll need to think about it for a bit or defer to Jakub.

It's the (new)  difference between how the following 3 OpenACC builtins handle 
asyn & wait args.
   case BUILT_IN_GOACC_PARALLEL:
      {
	set_oacc_fn_attrib (child_fn, clauses, &args);
	tagging = true;
      }
      /* FALLTHRU */
    case BUILT_IN_GOACC_ENTER_EXIT_DATA:
    case BUILT_IN_GOACC_UPDATE:

All 3 pass info about memory copies etc, async and optional waits.  For  E_E_D 
and UPDATE the async is always passed (with a special value for 'synchronous'), 
followed by a count and then variadic wait ints.

An alternarive I suppse would be to break out the meminfo arg pushing to a 
helper function and have 2 separate code paths, or something like that.

> Looks reasonable otherwise.

thanks for your review.  I'll repost shortly

nathan