On Wed, Mar 23, 2016 at 09:35:30AM +0100, Thomas Schwinge wrote:
> Hi!
>
> On Fri, 22 Jan 2016 12:50:38 -0600, James Norris <jnor...@codesourcery.com>
> wrote:
> > The attached patch fixes a defect reported with gcc 5.2
> > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69414).
> > It is also the case, the issue is present in the gomp4
> > branch. The patch also adds additional testing.
> >
> > Committed to gomp4 after bootstrap and regtesting.
>
> Jakub, is that following two-line patch (libgomp/oacc-mem.c, plus test
> cases) OK for trunk?
>
> > --- ChangeLog.gomp (revision 232740)
> > +++ ChangeLog.gomp (revision 232741)
> > @@ -1,3 +1,11 @@
> > +2016-01-22 James Norris <jnor...@codesourcery.com>
> > +
> > + * oacc-mem.c (delete_copyout, update_dev_host): Fix device address.
>
> Given that Daichi Fukuoka submitted the patch in
> <https://gcc.gnu.org/PR69414>, please acknowledge that in the ChangeLog.
> Please also include the test case submitted there; rename their file from
> gcc1.f90 to pr69414.f90 (or similar).
Yeah, the oacc-mem.c is a two liner, we don't need copyright assignment for
that, so please mention Daichi-san's name + mail address, and PR libgpmp/69414
in the ChangeLog entry.
> > --- oacc-mem.c (revision 232740)
> > +++ oacc-mem.c (revision 232741)
> > @@ -509,7 +509,7 @@
> > gomp_fatal ("[%p,%d] is not mapped", (void *)h, (int)s);
> > }
> >
> > - d = (void *) (n->tgt->tgt_start + n->tgt_offset);
> > + d = (void *) (n->tgt->tgt_start + n->tgt_offset + h - n->host_start);
I'd prefer (uintptr_t) h instead of, and you probably need to wrap it then.
> > host_size = n->host_end - n->host_start;
> >
> > @@ -562,7 +562,7 @@
> > gomp_fatal ("[%p,%d] is not mapped", h, (int)s);
> > }
> >
> > - d = (void *) (n->tgt->tgt_start + n->tgt_offset);
> > + d = (void *) (n->tgt->tgt_start + n->tgt_offset + h - n->host_start);
Ditto.
> > --- testsuite/libgomp.oacc-fortran/update-1-2.f90 (revision 0)
> > +++ testsuite/libgomp.oacc-fortran/update-1-2.f90 (revision 232741)
> > @@ -0,0 +1,239 @@
> > +! Copy of update-1.f90 with self exchanged with host for !$acc update
>
> Actually, that file contains additional changes.
>
> Anyway, I suggest we don't add new test cases with just self vs. host
> clauses exchanged -- their equivalence is something that should (already)
> be tested in GCC front end test cases. I know that we currently have a
> distinct libgomp.oacc-c-c++-common/update-1-2.c test case as a "copy of
> update-1.c with self exchanged with host for #pragma acc update", once
> added by me; I suggest we remove that. Let's just in the same file use a
> mixture of host and self clauses. (I'll take care of that.)
Otherwise LGTM, but please repost it with all the testcase changes you want
to make.
Jakub
