[Bug libgomp/92848] [OpenACC] Memory leak for simple 'acc_create', 'acc_delete' sequence

[tschwinge at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92848

--- Comment #8 from Thomas Schwinge  ---
Author: tschwinge
Date: Wed Dec 18 17:00:39 2019
New Revision: 279530

URL: https://gcc.gnu.org/viewcvs?rev=279530=gcc=rev
Log:
[PR92848] [OpenACC] Use 'GOMP_MAP_VARS_ENTER_DATA' for dynamic data lifetimes

libgomp/
PR libgomp/92848
* oacc-mem.c (acc_map_data, present_create_copy)
(goacc_insert_pointer): Use 'GOMP_MAP_VARS_ENTER_DATA'.
(acc_unmap_data, delete_copyout, goacc_remove_pointer): Adjust.
* testsuite/libgomp.oacc-c-c++-common/lib-50.c: Remove.
* testsuite/libgomp.oacc-c-c++-common/pr92848-1-d-a.c: New file
* testsuite/libgomp.oacc-c-c++-common/pr92848-1-d-p.c: Likewise.
* testsuite/libgomp.oacc-c-c++-common/pr92848-1-r-a.c: Likewise.
* testsuite/libgomp.oacc-c-c++-common/pr92848-1-r-p.c: Likewise.
* testsuite/libgomp.oacc-c-c++-common/subset-subarray-mappings-1-r-p.c:
Remove "XFAIL"s.

Added:
trunk/libgomp/testsuite/libgomp.oacc-c-c++-common/pr92848-1-d-a.c
trunk/libgomp/testsuite/libgomp.oacc-c-c++-common/pr92848-1-d-p.c
trunk/libgomp/testsuite/libgomp.oacc-c-c++-common/pr92848-1-r-a.c
trunk/libgomp/testsuite/libgomp.oacc-c-c++-common/pr92848-1-r-p.c
Removed:
trunk/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-50.c
Modified:
trunk/libgomp/ChangeLog
trunk/libgomp/oacc-mem.c
   
trunk/libgomp/testsuite/libgomp.oacc-c-c++-common/subset-subarray-mappings-1-r-p.c

[Bug libgomp/92848] [OpenACC] Memory leak for simple 'acc_create', 'acc_delete' sequence

[jules at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92848

--- Comment #6 from jules at gcc dot gnu.org ---
Created attachment 47479
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=47479=edit
Regressions with rc checking enabled

These are the OpenACC tests that regress with refcount checking enabled at
present.

[Bug libgomp/92848] [OpenACC] Memory leak for simple 'acc_create', 'acc_delete' sequence

[jules at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92848

jules at gcc dot gnu.org changed:

   What|Removed |Added

  Attachment #47451|0   |1
is obsolete||

--- Comment #7 from jules at gcc dot gnu.org ---
Created attachment 47480
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=47480=edit
Rebased patch for PR92848

Here's a rebased version of Thomas's patch. Any merge errors are my own!

[Bug libgomp/92848] [OpenACC] Memory leak for simple 'acc_create', 'acc_delete' sequence

[jules at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92848

--- Comment #5 from jules at gcc dot gnu.org ---
Created attachment 47478
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=47478=edit
Refcount checking for current trunk

Hi,

This is a merge of the refcount checking patch to current trunk, with the name
of the "virtual_refcount" field changed back to the current "dynamic_refcount".
With RC_CHECKING macro defined, this flags consistency errors in a large number
of OpenACC tests: how many of those are user-visible problems, I am not sure.
In at least some cases, we certainly have "two wrongs making a right" in that
the consistency errors do not result in a user-visible problem.

An assumption here is that "dynamic_refcount" is supposed to mean the same
thing as "virtual_refcount" does in the overhaul patch: those references that
represent dynamic data lifetimes without having links in the interlinked splay
tree/target_mem_desc structure. Or otherwise, the number which the static
refcount should be decreased by on encountering a "finalize" operation for the
data item in question.

Anyway: I hope this is useful in evaluating the rest of the refcount overhaul
patch.

[Bug libgomp/92848] [OpenACC] Memory leak for simple 'acc_create', 'acc_delete' sequence

[jules at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92848

--- Comment #4 from jules at gcc dot gnu.org ---
(In reply to Thomas Schwinge from comment #3)
> (In reply to jules from comment #2)
> > Again, please don't change this code under the feet of the refcount overhaul
> > patch!
> 
> But why?  This here is one independent chance.  The big "OpenACC reference
> count overhaul" will then either lose some of its changes, or replace this
> here by something else ('GOMP_MAP_VARS_OPENACC_ENTER_DATA'), but it's an
> incremental step (forward).

It's hard for me to evaluate if the changes are correct, given that I don't
trust the current baseline, mostly.

> > Using the (currently OpenMP-specific) GOMP_MAP_VARS_ENTER_DATA is
> > going to end up mighty confusing from OpenACC-specific code.
> 
> Why do you think so?  It has exactly! the semantics that we need here, for
> this PR.

Is the desired behaviour change just the reference count initialized by
target.c:gomp_map_vars_internal? I suppose that's OK. The danger is if the
semantics needed for OpenMP ever shift away from what we need for OpenACC here.
But I guess this code might not be around for long anyway?

[Bug libgomp/92848] [OpenACC] Memory leak for simple 'acc_create', 'acc_delete' sequence

[tschwinge at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92848

Thomas Schwinge  changed:

   What|Removed |Added

   Keywords||patch

--- Comment #3 from Thomas Schwinge  ---
(In reply to jules from comment #2)
> Again, please don't change this code under the feet of the refcount overhaul
> patch!

But why?  This here is one independent chance.  The big "OpenACC reference
count overhaul" will then either lose some of its changes, or replace this here
by something else ('GOMP_MAP_VARS_OPENACC_ENTER_DATA'), but it's an incremental
step (forward).


> Using the (currently OpenMP-specific) GOMP_MAP_VARS_ENTER_DATA is
> going to end up mighty confusing from OpenACC-specific code.

Why do you think so?  It has exactly! the semantics that we need here, for this
PR.

[Bug libgomp/92848] [OpenACC] Memory leak for simple 'acc_create', 'acc_delete' sequence

[jules at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92848

--- Comment #2 from jules at gcc dot gnu.org ---
Again, please don't change this code under the feet of the refcount overhaul
patch! Using the (currently OpenMP-specific) GOMP_MAP_VARS_ENTER_DATA is going
to end up mighty confusing from OpenACC-specific code.

[Bug libgomp/92848] [OpenACC] Memory leak for simple 'acc_create', 'acc_delete' sequence

[tschwinge at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92848

Thomas Schwinge  changed:

   What|Removed |Added

 Status|UNCONFIRMED |ASSIGNED
   Last reconfirmed||2019-12-09
   Assignee|unassigned at gcc dot gnu.org  |tschwinge at gcc dot 
gnu.org
 Ever confirmed|0   |1

--- Comment #1 from Thomas Schwinge  ---
Created attachment 47451
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=47451=edit
[WIP] [PR92848] [OpenACC] Use 'GOMP_MAP_VARS_ENTER_DATA' for dynamic data
lifetimes

Julian, your thoughts on the WIP patch I'm attaching, please?

As implied by
, using
'gomp_remove_var' instead of 'gomp_remove_var_async' is is not causing any
problems for nvptx offloading (strange?); does it for AMD GCN offloading?