[gomp4.5] Handle #pragma omp declare target link

[Jakub Jelinek]

On Fri, Nov 27, 2015 at 07:50:09PM +0300, Ilya Verbin wrote:
> On Thu, Nov 19, 2015 at 16:31:15 +0100, Jakub Jelinek wrote:
> > On Mon, Nov 16, 2015 at 06:40:43PM +0300, Ilya Verbin wrote:
> > > @@ -2009,7 +2010,8 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
> > >  	  decl = OMP_CLAUSE_DECL (c);
> > >  	  /* Global variables with "omp declare target" attribute
> > >  	     don't need to be copied, the receiver side will use them
> > > -	     directly.  */
> > > +	     directly.  However, global variables with "omp declare target link"
> > > +	     attribute need to be copied.  */
> > >  	  if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
> > >  	      && DECL_P (decl)
> > >  	      && ((OMP_CLAUSE_MAP_KIND (c) != GOMP_MAP_FIRSTPRIVATE_POINTER
> > > @@ -2017,7 +2019,9 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
> > >  		       != GOMP_MAP_FIRSTPRIVATE_REFERENCE))
> > >  		  || TREE_CODE (TREE_TYPE (decl)) == ARRAY_TYPE)
> > >  	      && is_global_var (maybe_lookup_decl_in_outer_ctx (decl, ctx))
> > > -	      && varpool_node::get_create (decl)->offloadable)
> > > +	      && varpool_node::get_create (decl)->offloadable
> > > +	      && !lookup_attribute ("omp declare target link",
> > > +				    DECL_ATTRIBUTES (decl)))
> > 
> > I wonder if Honza/Richi wouldn't prefer to have this info also
> > in cgraph, instead of looking up the attribute in each case.
> 
> So should I add a new flag into cgraph?
> Also it is used in gimplify_adjust_omp_clauses.

Richi said on IRC that lookup_attribute is ok, so let's keep it that way for
now.

> +	  /* Most significant bit of the size marks such vars.  */
> +	  unsigned HOST_WIDE_INT isize = tree_to_uhwi (size);
> +	  isize |= 1ULL << (int_size_in_bytes (const_ptr_type_node) * 8 - 1);

That supposedly should be BITS_PER_UNIT instead of 8.

> diff --git a/gcc/varpool.c b/gcc/varpool.c
> index 36f19a6..cbd1e05 100644
> --- a/gcc/varpool.c
> +++ b/gcc/varpool.c
> @@ -561,17 +561,21 @@ varpool_node::assemble_decl (void)
>       are not real variables, but just info for debugging and codegen.
>       Unfortunately at the moment emutls is not updating varpool correctly
>       after turning real vars into value_expr vars.  */
> +#ifndef ACCEL_COMPILER
>    if (DECL_HAS_VALUE_EXPR_P (decl)
>        && !targetm.have_tls)
>      return false;
> +#endif
>  
>    /* Hard register vars do not need to be output.  */
>    if (DECL_HARD_REGISTER (decl))
>      return false;
>  
> +#ifndef ACCEL_COMPILER
>    gcc_checking_assert (!TREE_ASM_WRITTEN (decl)
>  		       && TREE_CODE (decl) == VAR_DECL
>  		       && !DECL_HAS_VALUE_EXPR_P (decl));
> +#endif

This looks wrong, both of these clearly could affect anything with
DECL_HAS_VALUE_EXPR_P, not just the link vars.
So, if you need to handle the "omp declare target link" vars specially,
you should only handle those specially and nothing else.  And please try to
explain why.

> @@ -1005,13 +1026,18 @@ gomp_load_image_to_device (struct gomp_device_descr *devicep, unsigned version,
>    for (i = 0; i < num_vars; i++)
>      {
>        struct addr_pair *target_var = &target_table[num_funcs + i];
> -      if (target_var->end - target_var->start
> -	  != (uintptr_t) host_var_table[i * 2 + 1])
> +      uintptr_t target_size = target_var->end - target_var->start;
> +
> +      /* Most significant bit of the size marks "omp declare target link"
> +	 variables.  */
> +      bool is_link = target_size & (1ULL << (sizeof (uintptr_t) * 8 - 1));

__CHAR_BIT__ here instead of 8?

> @@ -1019,7 +1045,7 @@ gomp_load_image_to_device (struct gomp_device_descr *devicep, unsigned version,
>        k->host_end = k->host_start + (uintptr_t) host_var_table[i * 2 + 1];
>        k->tgt = tgt;
>        k->tgt_offset = target_var->start;
> -      k->refcount = REFCOUNT_INFINITY;
> +      k->refcount = is_link ? REFCOUNT_LINK : REFCOUNT_INFINITY;
>        k->async_refcount = 0;
>        array->left = NULL;
>        array->right = NULL;

Do we need to do anything in gomp_unload_image_from_device ?
I mean at least in questionable programs that for link vars don't decrement
the refcount of the var that replaced the link var to 0 first before
dlclosing the library.
At least host_var_table[j * 2 + 1] will have the MSB set, so we need to
handle it differently.  Perhaps for that case perform a lookup, and if we
get something which has link_map non-NULL, first perform as if there is
target exit data delete (var) on it first?

	Jakub