This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [gomp4.5] Handle #pragma omp declare target link
- From: Jakub Jelinek <jakub at redhat dot com>
- To: Ilya Verbin <iverbin at gmail dot com>
- Cc: Richard Biener <rguenther at suse dot de>, Jan Hubicka <hubicka at ucw dot cz>, gcc-patches at gcc dot gnu dot org, Kirill Yukhin <kirill dot yukhin at gmail dot com>
- Date: Mon, 30 Nov 2015 13:04:59 +0100
- Subject: Re: [gomp4.5] Handle #pragma omp declare target link
- Authentication-results: sourceware.org; auth=none
- References: <20150717130559 dot GI1780 at tucnak dot redhat dot com> <20151026183552 dot GD35077 at msticlxl57 dot ims dot intel dot com> <20151026190539 dot GX478 at tucnak dot redhat dot com> <20151026193904 dot GE35077 at msticlxl57 dot ims dot intel dot com> <20151026194940 dot GZ478 at tucnak dot redhat dot com> <20151116154043 dot GA18854 at msticlxl57 dot ims dot intel dot com> <20151119153115 dot GZ5675 at tucnak dot redhat dot com> <20151127165009 dot GA24771 at msticlxl57 dot ims dot intel dot com>
- Reply-to: Jakub Jelinek <jakub at redhat dot com>
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