Hi Chung-Lin!
On Fri, 14 Dec 2018 23:00:57 +0800, Chung-Lin Tang <chunglin_t...@mentor.com>
wrote:
> On 2018/12/14 10:53 PM, Thomas Schwinge wrote:
> > Please comment on the one TODO
> > which before your async re-work also was -- incorrectly? -- run
> > asynchronously?
> > @@ -563,6 +563,8 @@ GOACC_update (int device, size_t mapnum,
> > the value of the allocated device memory in the
> > previous pointer. */
> > *(uintptr_t *) hostaddrs[i] = (uintptr_t)dptr;
> > + /* This is intentionally no calling acc_update_device_async,
> > + because TODO. */
> > acc_update_device (hostaddrs[i], sizeof (uintptr_t));
> >
> > /* Restore the host pointer. */
| *(uintptr_t *) hostaddrs[i] = t;
>
> I don't remember adding this piece of comment, it might have been Cesar I
> guess?
That "TODO" comment, you mean? It was me who just added that one, to
highlight this, to ask you to comment, whether it's feasible to turn that
"acc_update_device" into "acc_update_device_async", too, or if that has
intentionally not been done, given that before your async re-work that
also was -- incorrectly? -- run asynchronously.
> I'm not sure if there's any real reason not to use acc_update_device_async
> here...
My worry was that the data object being copied here ("*hostaddrs[i]") is
changed immediatelly after the "acc_update_device" call (now inlined
above), so if that update get enqueued for asynchronous execution, it
might then actually copy the value after "Restore the host pointer".
Given the code before and after it, maybe "acc_update_device" is
generally the wrong function to call here? (That's, again, a separate
change, please.)
I have not yet researched where that code is coming from, and what it's
supposed to be used for. But as part of the async re-work we should at
least understand that one, too.
> Change and test to see?
Generally, when testing asynchronous behavior, I'd be wary of reading too
much into any such test results ("still PASSes" -- by chance?). Unless,
of course, there's then a clear regression somewhere ("now FAILs").
..., which there isn't, because adding a "gomp_fatal" in that code path,
that doesn't trigger a single time when running the libgomp testsuite...
Grüße
Thomas
