[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 <tschwinge at gcc dot gnu.org> 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 <tschwinge at gcc dot gnu.org> ---
(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, &cur_node);
> +	      if (n == NULL)
> +		{
> +		  gomp_mutex_unlock (&devicep->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?