Openacc launch API
Nathan Sidwell
nathan@acm.org
Thu Sep 17 13:24:00 GMT 2015
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
More information about the Gcc-patches
mailing list