Re: [PATCH] Adjust offsets for present data clauses

[Thomas Schwinge]

Hi Julian!

On Fri, 30 Nov 2018 10:37:09 +, Julian Brown  
wrote:
> > On 2018/7/21 6:07 AM, Cesar Philippidis wrote:
> > > This is another old gomp4 patch that corrects a bug where the
> > > runtime was passing the wrong offset for subarray data to the
> > > accelerator. The original description of this patch can be found
> > > here 

> This one will be superseded

Well, this is clearly a bug, so should be addressed independently.  I
just filed  "[OpenACC, libgomp] Adjust
offsets for present data clauses".  (Better late than never...)

> by the attach/detach patch, I think (where
> the additional offset is added also, via calling "gomp_map_val".

Aha, good to know: during review, I wondered about whether that function
should actually be used.  (But it also isn't in other places in
"libgomp/target.c".)


Grüße
 Thomas

Re: [PATCH] Adjust offsets for present data clauses

[Julian Brown]

On Fri, 30 Nov 2018 17:55:17 +0800
Chung-Lin Tang  wrote:

> On 2018/7/21 6:07 AM, Cesar Philippidis wrote:
> > This is another old gomp4 patch that corrects a bug where the
> > runtime was passing the wrong offset for subarray data to the
> > accelerator. The original description of this patch can be found
> > here 
> > 
> > I bootstrapped and regtested on x86_64/nvptx. Is it OK for trunk?
> > 
> > Thanks,
> > Cesar
> >   
> 
> Hi Thomas, this patch should be within your maintainership area now.
> 
> I think this patch is pretty obvious; this is what the 'offset' field
> of struct target_var_desc is supposed to be used for, and is in line
> with other sites throughout target.c.
> 
> I do think it might be better to use a more succinct form like as
> attached, you may consider which form better suits your taste when
> you apply it.

This one will be superseded by the attach/detach patch, I think (where
the additional offset is added also, via calling "gomp_map_val".

HTH,

Julian

Re: [PATCH] Adjust offsets for present data clauses

[Chung-Lin Tang]

On 2018/7/21 6:07 AM, Cesar Philippidis wrote:

This is another old gomp4 patch that corrects a bug where the runtime
was passing the wrong offset for subarray data to the accelerator. The
original description of this patch can be found here


I bootstrapped and regtested on x86_64/nvptx. Is it OK for trunk?

Thanks,
Cesar



Hi Thomas, this patch should be within your maintainership area now.

I think this patch is pretty obvious; this is what the 'offset' field
of struct target_var_desc is supposed to be used for, and is in line
with other sites throughout target.c.

I do think it might be better to use a more succinct form like as attached,
you may consider which form better suits your taste when you apply it.

Chung-Lin
Index: libgomp/oacc-parallel.c
===
--- libgomp/oacc-parallel.c (revision 266611)
+++ libgomp/oacc-parallel.c (working copy)
@@ -231,8 +231,11 @@ GOACC_parallel_keyed (int device, void (*fn) (void
 
   devaddrs = gomp_alloca (sizeof (void *) * mapnum);
   for (i = 0; i < mapnum; i++)
-devaddrs[i] = (void *) (tgt->list[i].key->tgt->tgt_start
-   + tgt->list[i].key->tgt_offset);
+devaddrs[i] = (tgt->list[i].key
+  ? (void *) (tgt->list[i].key->tgt->tgt_start
+  + tgt->list[i].key->tgt_offset
+  + tgt->list[i].offset)
+  : NULL);
 
   acc_dev->openacc.exec_func (tgt_fn, mapnum, hostaddrs, devaddrs,
  async, dims, tgt);