Re: [PATCH] openmp: Implicit 'declare target' for C++ static initializers

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Nov 19, 2020 at 06:07:28PM +0000, Kwok Cheung Yeung wrote:
> Even without this patch, G++ currently accepts something like

Sorry for the delay.

> int foo() { return 1; }
> int x = foo();
> #pragma omp declare target to(x)
> 
> but will not generate the device-side initializer for x, even though x is
> now present on the device. So this part of the implementation is broken with
> or without the patch.
> 
> Given that my patch doesn't make the current situation any worse, can I
> commit this portion of it to trunk for now, and leave device-side dynamic
> initialization for later?

Ok, but for the patch I have a few nits:

> +/* The C++ version of the get_decl_init langhook returns the static
> +   initializer for a variable declaration if present, otherwise it
> +   tries to find and return the dynamic initializer.  If not present,
> +   it returns NULL.  */
> +
> +static tree*
> +cxx_get_decl_init (tree decl)

The GCC coding style (appart from libstdc++) is type * rather than type*,
occurs several times in the patch.

> +{
> +  tree node;
> +
> +  if (DECL_INITIAL (decl))
> +    return &DECL_INITIAL (decl);
> +
> +  for (node = dynamic_initializers; node; node = TREE_CHAIN (node))
> +    if (TREE_VALUE (node) == decl)
> +      return &TREE_PURPOSE (node);

I'm worried with many dynamic initializers this will be worst case
quadratic.  Can't you use instead a hash map?  Note, as this is in the
FE, we might need to worry about PCH and GC.
Thus the hash map needs to be indexed by DECL_UIDs rather than pointers,
so perhaps use decl_tree_map?
Also, I'm worried that nothing releases dynamic_initializers (or the
decl_tree_map replacement).  We need it only during the discovery and not
afterwards, so it would be nice if the omp declare target discovery at the
end called another lang hook that would free the decl_tree_map, so that GC
can take it all.
If trees would remain there afterwards, we'd need to worry about destructive
gimplifier too and would need to unshare the dynamic initializers or
something.

I think it would be best to use omp_ in the hook name(s), and:
> --- a/gcc/cp/decl2.c
> +++ b/gcc/cp/decl2.c
> @@ -4940,6 +4940,11 @@ c_parse_final_cleanups (void)
>  	 loop.  */
>        vars = prune_vars_needing_no_initialization (&static_aggregates);
>  
> +      /* Copy the contents of VARS into DYNAMIC_INITIALIZERS.  */
> +      for (t = vars; t; t = TREE_CHAIN (t))
> +	dynamic_initializers = tree_cons (TREE_PURPOSE (t), TREE_VALUE (t),
> +					  dynamic_initializers);

Not to add there anything if (!flag_openmp).  We don't need to waste memory
when nobody is going to look at it.

	Jakub