Re: [PATCH] OpenACC reference count overhaul

[Thomas Schwinge <thomas at codesourcery dot com>]

Hi Julian!

On 2019-10-29T12:15:01+0000, Julian Brown <julian@codesourcery.com> wrote:
> This is a new version of the patch which hopefully addresses all review
> comments. Further commentary below.

Thanks, great, looking into that one -- I see you're removing more and
more special-case, strange code, replacing it with generic and/or
well-explained code.


Question, for my understanding:

> On Mon, 21 Oct 2019 16:14:11 +0200
> Thomas Schwinge <thomas@codesourcery.com> wrote:
>> On 2019-10-03T09:35:04-0700, Julian Brown <julian@codesourcery.com>
>> wrote:

>> > @@ -577,17 +551,14 @@ present_create_copy (unsigned f, void *h, size_t s, int async)  
>> 
>> > -      d = tgt->to_free;  
>> 
>> > +      n = lookup_host (acc_dev, h, s);
>> > +      assert (n != NULL);
>> > +      d = (void *) (n->tgt->tgt_start + n->tgt_offset + (uintptr_t) h
>> > +		    - n->host_start);  
>> 
>> |   return d;
>> 
>> Again, it's not obvious to me how that is semantically equivalent to
>> what we've returned before?
>
> This is a bug fix (it's mentioned in the ChangeLog).

Eh, well hidden.  Indeed that mentions:

	(present_create_copy): [...] Fix target pointer
	return value.

So that's not related to reference counting, needs to be discussed
separately.

..., and while I do agree that the current code is a bit "strange"
(returning 'tgt->to_free'), I couldn't quickly find or come up with a
test cases where this would actually do the wrong thing.  After all, this
is the code path taken for "not present", and 'tgt' is built anew for one
single mapping, with no alignment set (which would cause 'to_free' to
differ from 'tgt_start'); 'tgt_offset' should always be zero, and 'h'
always the same as 'host_start'.  What am I missing?  That is, given the
current set of libgomp test cases, the attached never triggeres.


Grüße
 Thomas


>From 2751a55293620a777e6a02587f36ab645b64ef0f Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Thu, 31 Oct 2019 19:09:47 +0100
Subject: [PATCH] libgomp/oacc-mem.c:present_create_copy: "not present"
 assertions

---
 libgomp/oacc-mem.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/libgomp/oacc-mem.c b/libgomp/oacc-mem.c
index 3773cdab85d..51a89e31ae5 100644
--- a/libgomp/oacc-mem.c
+++ b/libgomp/oacc-mem.c
@@ -1,3 +1,5 @@
+#pragma GCC optimize "-O0"
+
 /* OpenACC Runtime initialization routines
 
    Copyright (C) 2013-2019 Free Software Foundation, Inc.
@@ -589,6 +591,12 @@ present_create_copy (unsigned f, void *h, size_t s, int async)
       gomp_mutex_lock (&acc_dev->lock);
 
       d = tgt->to_free;
+      n = lookup_host (acc_dev, h, s);
+      assert (n);
+      assert (n->tgt_offset == 0);
+      assert (d == (void *) n->tgt->tgt_start);
+      assert (n->host_start == (uintptr_t) h);
+
       tgt->prev = acc_dev->openacc.data_environ;
       acc_dev->openacc.data_environ = tgt;
 
-- 
2.17.1