Openacc launch API

[Bernd Schmidt]

Since Jakub appears to be busy, I'll give my 2 cents.

On 08/25/2015 03:29 PM, Nathan Sidwell wrote:
> I  did rename the GOACC_parallel entry point to GOACC_parallel_keyed and
> provide a forwarding function. However, as the mkoffload data is
> incompatible, this is probably overkill.  I've had to increment the
> (just committed) version number to detect the change in data
> representation.  So any attempt to run an old binary with a new libgomp
> will fail at the loading point.

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?

> +/* Varadic launch arguments.  */
> +#define GOMP_LAUNCH_END 	0  /* End of args, no dev or op */
> +#define GOMP_LAUNCH_DIM		1  /* Launch dimensions, op = mask */
> +#define GOMP_LAUNCH_ASYNC	2  /* Async, op = cst val if not MAX  */
> +#define GOMP_LAUNCH_WAIT	3  /* Waits, op = num waits.  */
> +#define GOMP_LAUNCH_CODE_SHIFT	28
> +#define GOMP_LAUNCH_DEVICE_SHIFT 16
> +#define GOMP_LAUNCH_OP_SHIFT 0
> +#define GOMP_LAUNCH_PACK(CODE,DEVICE,OP)	\
> +  (((CODE) << GOMP_LAUNCH_CODE_SHIFT)		\
> +   | ((DEVICE) << GOMP_LAUNCH_DEVICE_SHIFT)	\
> +   | ((OP) << GOMP_LAUNCH_OP_SHIFT))
> +#define GOMP_LAUNCH_CODE(X) (((X) >> GOMP_LAUNCH_CODE_SHIFT) & 0xf)
> +#define GOMP_LAUNCH_DEVICE(X) (((X) >> GOMP_LAUNCH_DEVICE_SHIFT) & 0xfff)
> +#define GOMP_LAUNCH_OP(X) (((X) >> GOMP_LAUNCH_OP_SHIFT) & 0xffff)
> +#define GOMP_LAUNCH_OP_MAX 0xffff

I probably would have used something simpler, like a code/device/op 
argument triplet, but I guess this ok.

> -  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?

> +  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.

> +      assert (!GOMP_LAUNCH_DEVICE (tag));

Uh, that seems unfriendly, and not exactly forwards compatible. Can that 
fail a bit more gracefully? (Alternatively, implement the device_type 
stuff now so that we don't have TODOs in the code and don't have to 
worry about compatibility issues.)

> +  if (num_waits > 8)
> +    gomp_fatal ("Too many waits for legacy interface");

How did you arrive at this number?

>
> +GOACC_2.0,1 {
> +  global:
> +	GOACC_parallel_keyed;
> +} GOACC_2.0;

Did you mean to use a comma?

> +  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?

> +  for (comma = "", id = var_ids; id; comma = ",", id = id->next)
> +    fprintf (out, "%s\n\t%s", comma, id->ptx_name);

The comma trick is new to me, I'll have to remember this one.

> +static void
> +set_oacc_fn_attrib (tree fn, tree clauses, vec<tree> *args)
> +tree
> +get_oacc_fn_attrib (tree fn)

These need function comments.

> +{
> +  /* 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.

> @@ -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.

> +	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.

Looks reasonable otherwise.


Bernd