Hi Chung-Lin!
I realized: please add "PR libgomp/92840" to the Git commit log, as your
changes are directly a continuation of my earlier changes.
On 2024-03-05T01:18:28+0900, Chung-Lin Tang <clt...@pllab.cs.nthu.edu.tw> wrote:
> On 2023/10/31 11:06 PM, Thomas Schwinge wrote:
>>> @@ -691,15 +694,27 @@ goacc_exit_datum_1 (struct gomp_device_descr
>>> *acc_dev, void *h, size_t s,
>>>
>>> if (finalize)
>>> {
>>> - if (n->refcount != REFCOUNT_INFINITY)
>>> + if (n->refcount != REFCOUNT_INFINITY
>>> + && n->refcount != REFCOUNT_ACC_MAP_DATA)
>>> n->refcount -= n->dynamic_refcount;
>>> - n->dynamic_refcount = 0;
>>> +
>>> + if (n->refcount == REFCOUNT_ACC_MAP_DATA)
>>> + /* Mappings created by acc_map_data are returned to initial
>>> + dynamic_refcount of 1. Can only be deleted by acc_unmap_data. */
>>> + n->dynamic_refcount = 1;
>>> + else
>>> + n->dynamic_refcount = 0;
>>> }
>>> else if (n->dynamic_refcount)
>>> {
>>> - if (n->refcount != REFCOUNT_INFINITY)
>>> + if (n->refcount != REFCOUNT_INFINITY
>>> + && n->refcount != REFCOUNT_ACC_MAP_DATA)
>>> n->refcount--;
>>> - n->dynamic_refcount--;
>>> +
>>> + /* When mapping is created by acc_map_data, dynamic_refcount must be
>>> + maintained at >= 1. */
>>> + if (n->refcount != REFCOUNT_ACC_MAP_DATA || n->dynamic_refcount > 1)
>>> + n->dynamic_refcount--;
>>> }
>>
>> I'd find those changes more concise to understand if done the following
>> way: restore both 'if (finalize)' and 'else if (n->dynamic_refcount)'
>> branches to their original form (other than excluding 'n->refcount'
>> modification for 'REFCOUNT_ACC_MAP_DATA', as you have), and instead then
>> afterwards (that is, here), do:
>>
>> /* Mappings created by 'acc_map_data' can only be deleted by
>> 'acc_unmap_data'. */
>> if (n->refcount == REFCOUNT_ACC_MAP_DATA
>> && n->dynamic_refcount == 0)
>> n->dynamic_refcount = 1;
>>
>> That does have the same semantics, please verify?
>
> This does not have the same semantics, because if the original
> finalize/n->dynamic_refcount
> cases are left unmodified, they will treat REFCOUNT_ACC_MAP_DATA like a
> normal refcount and
> decrement n->refcount, and handling n->refcount == REFCOUNT_ACC_MAP_DATA
> later won't work either.
That's why I said: "restore [...] excluding 'n->refcount' modification
for 'REFCOUNT_ACC_MAP_DATA', as you have [...]". Sorry if that was
unclear.
> I have however, adjusted the nesting of cases to split the 'n->refcount ==
> REFCOUNT_ACC_MAP_DATA'
> case away. This should be easier to read.
Thanks, that easier to follow indeed. I had meant (on top of your v2):
--- a/libgomp/oacc-mem.c
+++ b/libgomp/oacc-mem.c
@@ -686,35 +686,27 @@ goacc_exit_datum_1 (struct gomp_device_descr
*acc_dev, void *h, size_t s,
gomp_fatal ("Dynamic reference counting assert fail\n");
}
- if (n->refcount == REFCOUNT_ACC_MAP_DATA)
+ if (finalize)
{
- if (finalize)
- {
- /* Mappings created by acc_map_data are returned to initial
- dynamic_refcount of 1. Can only be deleted by acc_unmap_data. */
- n->dynamic_refcount = 1;
- }
- else if (n->dynamic_refcount)
- {
- /* When mapping is created by acc_map_data, dynamic_refcount must be
- maintained at >= 1. */
- if (n->dynamic_refcount > 1)
- n->dynamic_refcount--;
- }
- }
- else if (finalize)
- {
- if (n->refcount != REFCOUNT_INFINITY)
+ if (n->refcount != REFCOUNT_INFINITY
+ && n->refcount != REFCOUNT_ACC_MAP_DATA)
n->refcount -= n->dynamic_refcount;
n->dynamic_refcount = 0;
}
else if (n->dynamic_refcount)
{
- if (n->refcount != REFCOUNT_INFINITY)
+ if (n->refcount != REFCOUNT_INFINITY
+ && n->refcount != REFCOUNT_ACC_MAP_DATA)
n->refcount--;
n->dynamic_refcount--;
}
+ /* Mappings created by 'acc_map_data' may only be deleted by
+ 'acc_unmap_data'. */
+ if (n->refcount == REFCOUNT_ACC_MAP_DATA
+ && n->dynamic_refcount == 0)
+ n->dynamic_refcount = 1;
+
if (n->refcount == 0)
{
bool copyout = (kind == GOMP_MAP_FROM
..., which really should have the same semantics? No strong opinion on
which of the two variants you now chose.
>>> @@ -480,7 +480,9 @@ gomp_increment_refcount (splay_tree_key k, htab_t
>>> *refcount_set)
>>>
>>> uintptr_t *refcount_ptr = &k->refcount;
>>>
>>> - if (REFCOUNT_STRUCTELEM_FIRST_P (k->refcount))
>>> + if (k->refcount == REFCOUNT_ACC_MAP_DATA)
>>> + refcount_ptr = &k->dynamic_refcount;
>>> + else if (REFCOUNT_STRUCTELEM_FIRST_P (k->refcount))
>>> refcount_ptr = &k->structelem_refcount;
>>> else if (REFCOUNT_STRUCTELEM_P (k->refcount))
>>> refcount_ptr = k->structelem_refcount_ptr;
>>> @@ -527,7 +529,9 @@ gomp_decrement_refcount (splay_tree_key k, htab_t
>>> *refcount_set, bool delete_p,
>>>
>>> uintptr_t *refcount_ptr = &k->refcount;
>>>
>>> - if (REFCOUNT_STRUCTELEM_FIRST_P (k->refcount))
>>> + if (k->refcount == REFCOUNT_ACC_MAP_DATA)
>>> + refcount_ptr = &k->dynamic_refcount;
>>> + else if (REFCOUNT_STRUCTELEM_FIRST_P (k->refcount))
>>> refcount_ptr = &k->structelem_refcount;
>>> else if (REFCOUNT_STRUCTELEM_P (k->refcount))
>>> refcount_ptr = k->structelem_refcount_ptr;
>>> @@ -560,6 +564,10 @@ gomp_decrement_refcount (splay_tree_key k, htab_t
>>> *refcount_set, bool delete_p,
>>> else if (*refcount_ptr > 0)
>>> *refcount_ptr -= 1;
>>>
>>> + /* Force back to 1 if this is an acc_map_data mapping. */
>>> + if (k->refcount == REFCOUNT_ACC_MAP_DATA && *refcount_ptr == 0)
>>> + *refcount_ptr = 1;
>>> +
>>> end:
>>> if (*refcount_ptr == 0)
>>> {
>>
>> It's not clear to me why you need this handling -- instead of just
>> handling 'REFCOUNT_ACC_MAP_DATA' like 'REFCOUNT_INFINITY' here, that is,
>> early 'return'?
>>
>> Per my understanding, this code is for OpenACC only exercised for
>> structured data regions, and it seems strange (unnecessary?) to adjust
>> the 'dynamic_refcount' for these for 'acc_map_data'-mapped data? Or am I
>> missing anything?
>
> No, that is not true. It goes through almost everything through
> gomp_map_vars_existing/_internal.
> This is what happens when you acc_create/acc_copyin on a mapping created by
> acc_map_data.
But I don't understand what you foresee breaking with the following (on
top of your v2):
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -476,14 +476,14 @@ gomp_free_device_memory (struct gomp_device_descr
*devicep, void *devptr)
static inline void
gomp_increment_refcount (splay_tree_key k, htab_t *refcount_set)
{
- if (k == NULL || k->refcount == REFCOUNT_INFINITY)
+ if (k == NULL
+ || k->refcount == REFCOUNT_INFINITY
+ || k->refcount == REFCOUNT_ACC_MAP_DATA)
return;
uintptr_t *refcount_ptr = &k->refcount;
- if (k->refcount == REFCOUNT_ACC_MAP_DATA)
- refcount_ptr = &k->dynamic_refcount;
- else if (REFCOUNT_STRUCTELEM_FIRST_P (k->refcount))
+ if (REFCOUNT_STRUCTELEM_FIRST_P (k->refcount))
refcount_ptr = &k->structelem_refcount;
else if (REFCOUNT_STRUCTELEM_P (k->refcount))
refcount_ptr = k->structelem_refcount_ptr;
@@ -522,7 +522,9 @@ static inline void
gomp_decrement_refcount (splay_tree_key k, htab_t *refcount_set, bool
delete_p,
bool *do_copy, bool *do_remove)
{
- if (k == NULL || k->refcount == REFCOUNT_INFINITY)
+ if (k == NULL
+ || k->refcount == REFCOUNT_INFINITY
+ || k->refcount == REFCOUNT_ACC_MAP_DATA)
{
*do_copy = *do_remove = false;
return;
@@ -530,9 +532,7 @@ gomp_decrement_refcount (splay_tree_key k, htab_t
*refcount_set, bool delete_p,
uintptr_t *refcount_ptr = &k->refcount;
- if (k->refcount == REFCOUNT_ACC_MAP_DATA)
- refcount_ptr = &k->dynamic_refcount;
- else if (REFCOUNT_STRUCTELEM_FIRST_P (k->refcount))
+ if (REFCOUNT_STRUCTELEM_FIRST_P (k->refcount))
refcount_ptr = &k->structelem_refcount;
else if (REFCOUNT_STRUCTELEM_P (k->refcount))
refcount_ptr = k->structelem_refcount_ptr;
@@ -565,10 +565,6 @@ gomp_decrement_refcount (splay_tree_key k, htab_t
*refcount_set, bool delete_p,
else if (*refcount_ptr > 0)
*refcount_ptr -= 1;
- /* Force back to 1 if this is an acc_map_data mapping. */
- if (k->refcount == REFCOUNT_ACC_MAP_DATA && *refcount_ptr == 0)
- *refcount_ptr = 1;
-
end:
if (*refcount_ptr == 0)
{
Can you please show a test case?
>> But please also to the "Minimal OpenACC variant corresponding to PR96668"
>> code in 'libgomp/oacc-mem.c:goacc_enter_data_internal' add a safeguard
>> that we're not running into 'REFCOUNT_ACC_MAP_DATA' there. I think
>> that's currently not (reasonably easily) possible, given that
>> 'acc_map_data' isn't available in OpenACC/Fortran, but it'll be available
>> later
('acc_map_data' now is available in OpenACC/Fortran as of Tobias' recent
commit r14-9196-g8b3f1edf9b38cb8a88c0a101a675d092bf6135d2
"OpenACC: Add Fortran routines
acc_{alloc,free,hostptr,deviceptr,memcpy_{to,from}_device*}".
See 'libgomp.oacc-fortran/acc_host_device_ptr.f90' for a simple test
case.)
>> and then I'd rather have an 'assert' trigger there, instead of
>> random behavior. (I'm not asking you to write a mixed OpenACC/Fortran
>> plus C test case for that scenario -- if feasible at all.)
>
> I am not really sure what you want me to do here, but REFCOUNT_ACC_MAP_DATA
> mappings
> are all created through a single GOMP_MAP_ALLOC kind. The complex stuff of
> MAP_STRUCT, MAP_TO_PSET, etc.
> should all be not related here (I presume even if Fortran eventually gets
> acc_map_data, it would be the
> compiler side which should take care of passing the raw
> data-pointer/array-size to the acc_map_data routine)
I see we already have:
if ((kinds[i] & 0xff) == GOMP_MAP_TO_PSET
&& tgt->list_count == 0)
{
/* 'declare target'. */
assert (n->refcount == REFCOUNT_INFINITY);
I think I wanted to you to add:
--- a/libgomp/oacc-mem.c
+++ b/libgomp/oacc-mem.c
@@ -1217,7 +1209,8 @@ goacc_enter_data_internal (struct gomp_device_descr
*acc_dev, size_t mapnum,
processed = true;
}
else
- assert (n->refcount != REFCOUNT_INFINITY);
+ assert (n->refcount != REFCOUNT_INFINITY
+ && n->refcount != REFCOUNT_ACC_MAP_DATA);
for (size_t j = 0; j < tgt->list_count; j++)
if (tgt->list[j].key == n)
Please check these items, and then we're good to go.
Grüße
Thomas
> libgomp/ChangeLog:
>
> * libgomp.h (REFCOUNT_ACC_MAP_DATA): Define as (REFCOUNT_SPECIAL | 2).
> * oacc-mem.c (acc_map_data): Adjust to use REFCOUNT_ACC_MAP_DATA,
> initialize dynamic_refcount as 1.
> (acc_unmap_data): Adjust to use REFCOUNT_ACC_MAP_DATA, remove TODO
> comments. Add assert of 'n->dynamic_refcount >= 1' and comments.
> (goacc_map_var_existing): Add REFCOUNT_ACC_MAP_DATA case.
> (goacc_exit_datum_1): Add REFCOUNT_ACC_MAP_DATA case, respect
> REFCOUNT_ACC_MAP_DATA when decrementing/finalizing. Force lowest
> dynamic_refcount to be 1 for REFCOUNT_ACC_MAP_DATA.
> (goacc_enter_data_internal): Add REFCOUNT_ACC_MAP_DATA case.
> * target.c (gomp_increment_refcount): Add REFCOUNT_ACC_MAP_DATA case.
> (gomp_decrement_refcount): Add REFCOUNT_ACC_MAP_DATA case, force lowest
> dynamic_refcount to be 1 for REFCOUNT_ACC_MAP_DATA.
> * testsuite/libgomp.oacc-c-c++-common/lib-96.c: New testcase.
> * testsuite/libgomp.oacc-c-c++-common/unmap-infinity-1.c: Adjust
> testcase error output scan test.
>
>
> diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
> index f98cccd8b66..089393846d1 100644
> --- a/libgomp/libgomp.h
> +++ b/libgomp/libgomp.h
> @@ -1163,6 +1163,8 @@ struct target_mem_desc;
> /* Special value for refcount - tgt_offset contains target address of the
> artificial pointer to "omp declare target link" object. */
> #define REFCOUNT_LINK (REFCOUNT_SPECIAL | 1)
> +/* Special value for refcount - created through acc_map_data. */
> +#define REFCOUNT_ACC_MAP_DATA (REFCOUNT_SPECIAL | 2)
>
> /* Special value for refcount - structure element sibling list items.
> All such key refounts have REFCOUNT_STRUCTELEM bits set, with _FLAG_FIRST
> diff --git a/libgomp/oacc-mem.c b/libgomp/oacc-mem.c
> index ba48a1ccbb7..ffd56e397f6 100644
> --- a/libgomp/oacc-mem.c
> +++ b/libgomp/oacc-mem.c
> @@ -411,7 +411,8 @@ acc_map_data (void *h, void *d, size_t s)
> assert (n->refcount == 1);
> assert (n->dynamic_refcount == 0);
> /* Special reference counting behavior. */
> - n->refcount = REFCOUNT_INFINITY;
> + n->refcount = REFCOUNT_ACC_MAP_DATA;
> + n->dynamic_refcount = 1;
>
> if (profiling_p)
> {
> @@ -455,12 +456,7 @@ acc_unmap_data (void *h)
> gomp_fatal ("[%p,%d] surrounds %p",
> (void *) n->host_start, (int) host_size, (void *) h);
> }
> - /* TODO This currently doesn't catch 'REFCOUNT_INFINITY' usage different
> from
> - 'acc_map_data'. Maybe 'dynamic_refcount' can be used for disambiguating
> - the different 'REFCOUNT_INFINITY' cases, or simply separate
> - 'REFCOUNT_INFINITY' values per different usage ('REFCOUNT_ACC_MAP_DATA'
> - etc.)? */
> - else if (n->refcount != REFCOUNT_INFINITY)
> + else if (n->refcount != REFCOUNT_ACC_MAP_DATA)
> {
> gomp_mutex_unlock (&acc_dev->lock);
> gomp_fatal ("refusing to unmap block [%p,+%d] that has not been mapped"
> @@ -468,13 +464,12 @@ acc_unmap_data (void *h)
> (void *) h, (int) host_size);
> }
>
> - struct target_mem_desc *tgt = n->tgt;
> + /* This should hold for all mappings created by acc_map_data. We are
> however
> + removing this mapping in a "finalize" manner, so dynamic_refcount > 1
> does
> + not matter. */
> + assert (n->dynamic_refcount >= 1);
>
> - if (tgt->refcount == REFCOUNT_INFINITY)
> - {
> - gomp_mutex_unlock (&acc_dev->lock);
> - gomp_fatal ("cannot unmap target block");
> - }
> + struct target_mem_desc *tgt = n->tgt;
>
> /* Above, we've verified that the mapping must have been set up by
> 'acc_map_data'. */
> @@ -519,7 +514,8 @@ goacc_map_var_existing (struct gomp_device_descr
> *acc_dev, void *hostaddr,
> }
>
> assert (n->refcount != REFCOUNT_LINK);
> - if (n->refcount != REFCOUNT_INFINITY)
> + if (n->refcount != REFCOUNT_INFINITY
> + && n->refcount != REFCOUNT_ACC_MAP_DATA)
> n->refcount++;
> n->dynamic_refcount++;
>
> @@ -683,13 +679,30 @@ goacc_exit_datum_1 (struct gomp_device_descr *acc_dev,
> void *h, size_t s,
>
> assert (n->refcount != REFCOUNT_LINK);
> if (n->refcount != REFCOUNT_INFINITY
> + && n->refcount != REFCOUNT_ACC_MAP_DATA
> && n->refcount < n->dynamic_refcount)
> {
> gomp_mutex_unlock (&acc_dev->lock);
> gomp_fatal ("Dynamic reference counting assert fail\n");
> }
>
> - if (finalize)
> + if (n->refcount == REFCOUNT_ACC_MAP_DATA)
> + {
> + if (finalize)
> + {
> + /* Mappings created by acc_map_data are returned to initial
> + dynamic_refcount of 1. Can only be deleted by acc_unmap_data. */
> + n->dynamic_refcount = 1;
> + }
> + else if (n->dynamic_refcount)
> + {
> + /* When mapping is created by acc_map_data, dynamic_refcount must be
> + maintained at >= 1. */
> + if (n->dynamic_refcount > 1)
> + n->dynamic_refcount--;
> + }
> + }
> + else if (finalize)
> {
> if (n->refcount != REFCOUNT_INFINITY)
> n->refcount -= n->dynamic_refcount;
> @@ -1131,7 +1144,8 @@ goacc_enter_data_internal (struct gomp_device_descr
> *acc_dev, size_t mapnum,
> }
> /* This is a special case because we must increment the refcount by
> the number of mapped struct elements, rather than by one. */
> - if (n->refcount != REFCOUNT_INFINITY)
> + if (n->refcount != REFCOUNT_INFINITY
> + && n->refcount != REFCOUNT_ACC_MAP_DATA)
> n->refcount += groupnum - 1;
> n->dynamic_refcount += groupnum - 1;
> }
> diff --git a/libgomp/target.c b/libgomp/target.c
> index 1367e9cce6c..1851feba271 100644
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -481,7 +481,9 @@ gomp_increment_refcount (splay_tree_key k, htab_t
> *refcount_set)
>
> uintptr_t *refcount_ptr = &k->refcount;
>
> - if (REFCOUNT_STRUCTELEM_FIRST_P (k->refcount))
> + if (k->refcount == REFCOUNT_ACC_MAP_DATA)
> + refcount_ptr = &k->dynamic_refcount;
> + else if (REFCOUNT_STRUCTELEM_FIRST_P (k->refcount))
> refcount_ptr = &k->structelem_refcount;
> else if (REFCOUNT_STRUCTELEM_P (k->refcount))
> refcount_ptr = k->structelem_refcount_ptr;
> @@ -528,7 +530,9 @@ gomp_decrement_refcount (splay_tree_key k, htab_t
> *refcount_set, bool delete_p,
>
> uintptr_t *refcount_ptr = &k->refcount;
>
> - if (REFCOUNT_STRUCTELEM_FIRST_P (k->refcount))
> + if (k->refcount == REFCOUNT_ACC_MAP_DATA)
> + refcount_ptr = &k->dynamic_refcount;
> + else if (REFCOUNT_STRUCTELEM_FIRST_P (k->refcount))
> refcount_ptr = &k->structelem_refcount;
> else if (REFCOUNT_STRUCTELEM_P (k->refcount))
> refcount_ptr = k->structelem_refcount_ptr;
> @@ -561,6 +565,10 @@ gomp_decrement_refcount (splay_tree_key k, htab_t
> *refcount_set, bool delete_p,
> else if (*refcount_ptr > 0)
> *refcount_ptr -= 1;
>
> + /* Force back to 1 if this is an acc_map_data mapping. */
> + if (k->refcount == REFCOUNT_ACC_MAP_DATA && *refcount_ptr == 0)
> + *refcount_ptr = 1;
> +
> end:
> if (*refcount_ptr == 0)
> {
> diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-96.c
> b/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-96.c
> new file mode 100644
> index 00000000000..7bc55b94f33
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-96.c
> @@ -0,0 +1,36 @@
> +/* { dg-do run } */
> +/* { dg-skip-if "" { *-*-* } { "-DACC_MEM_SHARED=1" } } */
> +
> +/* Test if acc_unmap_data has implicit finalize semantics. */
> +
> +#include <stdlib.h>
> +#include <openacc.h>
> +
> +int
> +main (int argc, char **argv)
> +{
> + const int N = 256;
> + unsigned char *h;
> + void *d;
> +
> + h = (unsigned char *) malloc (N);
> +
> + d = acc_malloc (N);
> +
> + acc_map_data (h, d, N);
> +
> + acc_copyin (h, N);
> + acc_copyin (h, N);
> + acc_copyin (h, N);
> +
> + acc_unmap_data (h);
> +
> + if (acc_is_present (h, N))
> + abort ();
> +
> + acc_free (d);
> +
> + free (h);
> +
> + return 0;
> +}
> diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/unmap-infinity-1.c
> b/libgomp/testsuite/libgomp.oacc-c-c++-common/unmap-infinity-1.c
> index 872f0c1de5c..9ed9fa7e413 100644
> --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/unmap-infinity-1.c
> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/unmap-infinity-1.c
> @@ -10,7 +10,7 @@ main (int argc, char *argv[])
> {
> acc_init (acc_device_default);
> acc_unmap_data ((void *) foo);
> -/* { dg-output "libgomp: cannot unmap target block" } */
> +/* { dg-output "libgomp: refusing to unmap block
> \\\[\[0-9a-fA-FxX\]+,\\\+64\\\] that has not been mapped by 'acc_map_data'" }
> */
> return 0;
> }
>
