OpenACC ICV acc-default-async-var

[Chung-Lin Tang]

Hi Thomas,
actually the current version of the acc_get/set_default_async patch is
combined into:
https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01426.html

This patch you're referring here was a version from early 2017.

I'll try to reply to the still applying comments here below.

On 2018/11/18 10:36 AM, Thomas Schwinge wrote:
>> --- include/gomp-constants.h	(revision 245382)
>> +++ include/gomp-constants.h	(working copy)
> 
>>   /* Asynchronous behavior.  Keep in sync with
>>      libgomp/{openacc.h,openacc.f90,openacc_lib.h}:acc_async_t.  */
>>   
>> +#define GOMP_ASYNC_DEFAULT		0
>>   #define GOMP_ASYNC_NOVAL		-1
>>   #define GOMP_ASYNC_SYNC			-2
> 
> This means that "acc_set_default_async(acc_async_default)" will set
> acc-default-async-var to "0", that is, the same as
> "acc_set_default_async(0)".  It thus follows that
> "async"/"async(acc_async_noval)" is the same as "async(0)".  Is that
> intentional?
> 
> It is in line with the OpenACC 2.5 specification: "initial value [...] is
> implementation defined", but I wonder why map it to "async(0)", and not
> to its own, unspecified, but separate queue.  In the latter case,
> "acc_async_default" etc. would then map to a negative value to denote
> this unspecified, but separate queue (and your changes would need to be
> adapted for that).
> 
> I have not verified whether we're currently already having (on trunk
> and/or openacc-gcc-8-branch) the semantics of the queue of
> "async(acc_async_noval)" mapping to the same queue as "async(0)"?

As long as the thr->default_async variable == 0 (as it is initially)
then async(acc_async_noval) maps to async(0).

> I'm fine to accept your changes as proposed (basically, everthing from
> your patch posted that has a "default_async" in it), for that's an
> incremental improvement anyway.  But -- unless you tell me I've
> misunderstood something -- I'll get the issue I raised clarified with the
> OpenACC technical committee, and we will then later improve this further.
> 
> No matter what the outcome, the implementation-defined behavior should be
> documented.  (Can do that once we get the intentions clarified.)

Well, the current submitted implementation of async queues manages an array of them
for each thread. So the intuitive default queue is the first (index 0), and
to support reverting to default when accepting 'acc_async_default' as an argument,
defining acc_async_default == 0 is the logical choice.

The 'default' async is not symbolically a specific queue, it is simply a thread-local
variable for what is referred by default when 'acc_async_noval' is encountered.
 From that sense, initializing it as some negative integer doesn't make sense

Of course, if really desired, we implement the "default default" to be an alternative
queue separate from the non-negative queue space, but I feel this is overkill.

>> +void
>> +acc_set_default_async (int async)
>> +{
>> +  if (async < acc_async_sync)
>> +    gomp_fatal ("invalid async argument: %d", async);
> 
> (This will nowadays use "async_valid_stream_id_p" or some such.)

Okay, I'll revise this part if needed. Although I am not sure if such a
specific check is really needed in the new async code, because most
(if not all) checking is centralized when indexing into the goacc_asyncqueue
array (and NULL is returned if error).

>> +  thr->default_async = async;
>> +}
> 
>> --- libgomp/oacc-plugin.c	(revision 245382)
>> +++ libgomp/oacc-plugin.c	(working copy)
> 
>> +/* Return the default async number from the TLS data for the current thread.  */
>> +
>> +int
>> +GOMP_PLUGIN_acc_thread_default_async (void)
>> +{
>> +  struct goacc_thread *thr = goacc_thread ();
>> +  return thr ? thr->default_async : acc_async_default;
>> +}
> 
> As I understand, the need for this function will disappear with your
> later "async re-work" changes, so OK as posted, but I wondered in which
> cases we would not have a valid "goacc_thread" when coming here?  (Might
> again related to the "goacc_lazy_initialize" issue mentioned above.)

Yes, this thing was deleted in the final upstream async rework submission.
Any further questions need not apply, the new way is entirely different.
This plugin routine was kind of like an artifact of inappropriate layering of logic :)

>> --- libgomp/testsuite/libgomp.oacc-c-c++-common/asyncwait-2.c	(revision 0)
>> +++ libgomp/testsuite/libgomp.oacc-c-c++-common/asyncwait-2.c	(revision 0)
>> @@ -0,0 +1,904 @@
>> +[...]
>> +    acc_set_default_async (s);
>> +[...]
> 
> This is the one single test case using this functionality, but it only
> verifies "correct results', but doesn't observe the actual queues (for
> example, CUDA streams) being used.
> 
> Need test cases for "acc_get_default_async", too, and also Fortran ones.

We'll supplement more testcases later.

> Generally, I envision test cases running a few "acc_get_cuda_stream"
> calls with relevant argument values, to see whether the expected
> queues/streames are being used.  (Similar for other offload targets.)
> 
> But I suppose we might again need to get clarified whether
> "acc_get_cuda_stream(acc_async_sync)",
> "acc_get_cuda_stream(acc_async_noval)", or
> "acc_get_cuda_stream(acc_async_default)" are actually valid calls (given
> that these argument values are not valid "async value"s), and these would
> then return the respective CUDA stream handles, different from the one
> returned for "acc_get_cuda_stream(0)" etc.
> 
> That said, we can certainly implement it that way, because that's not
> against the specification.

I think the likely clarification we'll ever get on this is that it's
implementation defined :P

Thanks,
Chung-Lin