This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] OpenACC reference count overhaul
- From: Thomas Schwinge <thomas at codesourcery dot com>
- To: Julian Brown <julian at codesourcery dot com>
- Cc: Jakub Jelinek <jakub at redhat dot com>, <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 31 Oct 2019 19:11:57 +0100
- Subject: Re: [PATCH] OpenACC reference count overhaul
- Ironport-sdr: 0A1VvLB9ohuaVVx9/tYZAJmJWEiKiW4SMaq9Bpj4cVLJznrR2NKnKo9zMJh1BV1spHHk19V3zd AHLbsMG22WfCpj00as0B0vLYzXNshnggjJS96A5DRZ5lBXmZ8FRFgezIHudkVyHnwkBCNCPOLb xjWpZC5BKnooXuGvbMhXV8+LBYaO3bHGeon9TaTIiBJ5AZi9nmB1dHzmwnc7TIlwrriviYs8e4 3ySNVNf2r8KO5xHbtpywmPijfsPMY8ThSAYhEb8MQ21UPcKbuQFcn6/yOdR9rX/f8ba1A3nyXg jlg=
- Ironport-sdr: X4gyG75G//avmHxlJsKd3hqy/1IMCOkFlL/NkO863wcgFfU8x5FNQ3G9INn1Rv56rdV9AFxQcJ Y2hYfgJBPMrhFS5cTAs6SP2e0H76WjiJoTpUDpC5W+qvPrJ5nCRtcAWdoeUwNLg4GoOFDjiFWv ZU7XeDaps4js6Mj3/3YLiEgUTrTrfqjio8VSPF2Y8KbBYXiN8om/lIIKCgteITkc6FZ+Cw981z lFcEB7JhG8gi1dlFNcg3PpNTMtqcoNMo2RRpxJpS47KxVCeQUtpWCuK742nxXaqH/y/GNr0pTp hUk=
- References: <20191003163505.49997-2-julian@codesourcery.com> <87r236tccs.fsf@euler.schwinge.homeip.net> <20191029121501.7652b92b@squid.athome>
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