[Bug libgomp/92028] OpenACC 'host_data' execution test regressions with nvptx offloading

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92028

--- Comment #4 from Jakub Jelinek  ---
Author: jakub
Date: Wed Oct  9 07:33:02 2019
New Revision: 276753

URL: https://gcc.gnu.org/viewcvs?rev=276753=gcc=rev
Log:
PR libgomp/92028
* target.c (gomp_map_vars_internal): Readd the previous
GOMP_MAP_USE_DEVICE_PTR handling code in the first loop,
though do that just in the !not_found_cnt case.

Modified:
trunk/libgomp/ChangeLog
trunk/libgomp/target.c

[Bug libgomp/92028] OpenACC 'host_data' execution test regressions with nvptx offloading

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92028

--- Comment #3 from Jakub Jelinek  ---
Created attachment 47007
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=47007=edit
gcc10-pr92028.patch

Updated patch with a longish comment.

[Bug libgomp/92028] OpenACC 'host_data' execution test regressions with nvptx offloading

[tschwinge at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92028

Thomas Schwinge  changed:

   What|Removed |Added

 Status|UNCONFIRMED |ASSIGNED
   Last reconfirmed||2019-10-08
   Assignee|unassigned at gcc dot gnu.org  |jakub at gcc dot gnu.org
 Ever confirmed|0   |1

--- Comment #2 from Thomas Schwinge  ---
(In reply to Jakub Jelinek from comment #1)
> Looking at that commit, what I've commited in target.c is certainly not what
> I meant to commit, which was something like (untested):

Thanks, that does resolves the issue.  :-)


Now, one very general comment:

> --- libgomp/target.c.jj   2019-10-07 13:09:07.038253353 +0200
> +++ libgomp/target.c  2019-10-08 15:19:16.249439849 +0200
@@ -593,6 +593,19 @@ gomp_map_vars_internal (struct gomp_devi
> tgt->list[i].key = NULL;
> if (!not_found_cnt)
>   {
> +   cur_node.host_start = (uintptr_t) hostaddrs[i];
> +   cur_node.host_end = cur_node.host_start;
> +   splay_tree_key n = gomp_map_lookup (mem_map, _node);
> +   if (n == NULL)
> + {
> +   gomp_mutex_unlock (>lock);
> +   gomp_fatal ("use_device_ptr pointer wasn't mapped");
> + }
> +   cur_node.host_start -= n->host_start;
> +   hostaddrs[i]
> + = (void *) (n->tgt->tgt_start + n->tgt_offset
> + + cur_node.host_start);
> +   tgt->list[i].offset = ~(uintptr_t) 0;
>   }
> else
>   tgt->list[i].offset = 0;

So, with some careful thinking/digging one is of course able to extract:

> Essentially, if nothing is so far queued to be mapped, we can try to resolve
> USE_DEVICE_PTR the way we did before, but if there is something queued,
> because the middle-end for OpenMP 5.0 ensures that map clauses go before
> use_device_* clauses, we need to wait and handle it only later.

... such a rationale out of the actual GCC source code -- but what's the reason
that you're not adding such comments directly to the code?

Not just here, but generally I find that your patch submission emails and PR
comments very often contain very valuable information, but that's just in email
but not in the actual GCC source code -- so that information is not readily
accessible when reading the code.  Which is a pity.

Can I suggest that we all -- including you :-P -- help to make this complex OMP
gimplification/lowering/expanding/libgomp code better understandable and
maintainable by documenting it better?

[Bug libgomp/92028] OpenACC 'host_data' execution test regressions with nvptx offloading

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92028

--- Comment #1 from Jakub Jelinek  ---
Looking at that commit, what I've commited in target.c is certainly not what I
meant to commit, which was something like (untested):
--- libgomp/target.c.jj 2019-10-07 13:09:07.038253353 +0200
+++ libgomp/target.c2019-10-08 15:19:16.249439849 +0200
@@ -593,6 +593,19 @@ gomp_map_vars_internal (struct gomp_devi
  tgt->list[i].key = NULL;
  if (!not_found_cnt)
{
+ cur_node.host_start = (uintptr_t) hostaddrs[i];
+ cur_node.host_end = cur_node.host_start;
+ splay_tree_key n = gomp_map_lookup (mem_map, _node);
+ if (n == NULL)
+   {
+ gomp_mutex_unlock (>lock);
+ gomp_fatal ("use_device_ptr pointer wasn't mapped");
+   }
+ cur_node.host_start -= n->host_start;
+ hostaddrs[i]
+   = (void *) (n->tgt->tgt_start + n->tgt_offset
+   + cur_node.host_start);
+ tgt->list[i].offset = ~(uintptr_t) 0;
}
  else
tgt->list[i].offset = 0;

Essentially, if nothing is so far queued to be mapped, we can try to resolve
USE_DEVICE_PTR the way we did before, but if there is something queued, because
the middle-end for OpenMP 5.0 ensures that map clauses go before use_device_*
clauses, we need to wait and handle it only later.