Re: [Patch] OpenMP: Add C++ support for 'omp allocate' with stack variables

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Oct 20, 2023 at 06:49:58PM +0200, Tobias Burnus wrote:
> +      if (!processing_template_decl)
> +       finish_omp_allocate (true, OMP_CLAUSE_LOCATION (nl), var);

The above should be called even if processing_template_decl, see below.
And pass DECL_ATTRIBUTES (var) to it (also see below).

> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7801,6 +7801,10 @@ extern tree finish_omp_for			(location_t, enum tree_code,
>  						 tree, tree, tree, tree, tree,
>  						 tree, tree, vec<tree> *, tree);
>  extern tree finish_omp_for_block		(tree, tree);
> +extern void finish_omp_allocate			(bool, location_t, tree,
> +						 tree = NULL_TREE,
> +						 tsubst_flags_t = tf_warning_or_error,
> +						 tree = NULL_TREE);

For a function called in 2 spots adding default arguments is an overkill,
but see below.

> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -41864,6 +41864,67 @@ cp_parser_omp_structured_block (cp_parser *parser, bool *if_p)
>    return finish_omp_structured_block (stmt);
>  }
>  

Better also add a comment about this structure.

> +struct cp_omp_loc_tree
> +{
> +  location_t loc;
> +  tree var;
> +};
> +
> +/* Check whether the expression used in the allocator clause is declared or
> +   modified between the variable declaration and its allocate directive.  */
> +static tree
> +cp_check_omp_allocate_allocator_r (tree *tp, int *, void *data)
> +{
> +  tree var = ((struct cp_omp_loc_tree *) data)->var;
> +  location_t loc = ((struct cp_omp_loc_tree *) data)->loc;
> +  tree v = NULL_TREE;
> +  if (TREE_CODE (*tp) == VAR_DECL)

VAR_P ?

> +    for (v = current_binding_level->names; v; v = TREE_CHAIN (v))
> +      if (v == var)
> +	break;

> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 210c6cb9e4d..10603f4c39f 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -18379,6 +18379,10 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t complain, tree in_decl)
>  
>  		    cp_finish_decl (decl, init, const_init, asmspec_tree, 0,
>  				    decomp);
> +	
> +		    if (flag_openmp && VAR_P (decl))
> +		      finish_omp_allocate (false, DECL_SOURCE_LOCATION (decl),
> +					   decl, args, complain, in_decl);

The normal C++ FE way of doing stuff is perform all the substitutions
here (in tsubst_stmt, tsubst_expr and functions it calls) and then call
the various semantics.cc etc. finalizers, with already tsubsted arguments.
So, this would be the first time to do it differently.
IMHO better lookup_attribute here, if found tsubst what is needed and
only then call finish_omp_allocate.  Ideally when you've done the work
already pass the attr to the function as well.

>  		    if (ndecl != error_mark_node)
>  		      cp_finish_decomp (ndecl, decomp);
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index dc3c11461fb..30c70c0b13e 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -10987,6 +10987,86 @@ finish_omp_for_block (tree bind, tree omp_for)
>    return bind;
>  }
>  

Please add a function comment.

> +void
> +finish_omp_allocate (bool in_parsing, location_t loc, tree decl, tree args,
> +		     tsubst_flags_t complain, tree in_decl)

As mentioned above, please drop the in_parsing, args, complain and in_decl
arguments and add attr.

> +{
> +  location_t loc2;
> +  tree attr = lookup_attribute ("omp allocate", DECL_ATTRIBUTES (decl));
> +  if (attr == NULL_TREE)
> +    return;
> +
> +  tree allocator = TREE_PURPOSE (TREE_VALUE (attr));
> +  tree alignment = TREE_VALUE (TREE_VALUE (attr));
> +
> +  if (alignment == error_mark_node)
> +    TREE_VALUE (TREE_VALUE (attr)) = NULL_TREE;
> +  else if (alignment)
> +    {
> +      location_t loc2 = EXPR_LOCATION (alignment);
> +      if (!in_parsing)
> +	alignment = tsubst_expr (alignment, args, complain, in_decl);
> +      alignment = fold_non_dependent_expr (alignment);
> +
> +      if (TREE_CODE (alignment) != INTEGER_CST
> +	  || !INTEGRAL_TYPE_P (TREE_TYPE (alignment))

Please see e.g. the r13-8124 and r14-6193 changes.  Unless we have (possibly
violating standard?) hard restriction like we have on the collapse and
ordered clauses where we want the argument to be INTEGER_CST with the right
value already during parsing because parsing depends on it, we want to
handle 3 different cases, the directive appearing in non-template code,
in that case the diagnostics should be done right away, or in template code
where the arguments of the clauses etc. are not dependent (type nor value),
in that case we want the diagnostics to be also done at parsing time; not
strictly required by the standard, but QoI, get diagnostics of something
clearly incorrect in a template even when the template will never be
instantiated.  And finally if it is dependent (value dependent like say in
template <int N, typename T, T X>
N, in that case we can diagnose at parsing time if the type is not integer
type, but shouldn't diagnose anything about the value, or say for X
above which is type dependent, then we shouldn't diagnose anything for it).
Now, typically the finish_* and similar routines in semantics.cc etc.
for processing_template_decl construct some tree that is later tsubsted,
and often if it is just value dependent and not type dependent ensure
it has the right finalized type; that part isn't needed in this case,
when it is already the parser caller (and should be the pt.cc caller) which
arranges stuff to be in the attribute.

> +	  || tree_int_cst_sgn (alignment) != 1
> +	  || !integer_pow2p (alignment))

So, you want to just store the alignment into TREE_VALUE (TREE_VALUE (attr))
if it is type_dependent_expression_p so that later instantiation handles
that, or error if it is !INTEGRAL_TYPE_P otherwise.  Otherwise
if value_dependent_expression_p again just store it, or otherwise
do the INTEGER_CST and exact value checks and error otherwise.

> +	{
> +	  error_at (loc2, "%<align%> clause argument needs to be positive "
> +			  "constant power of two integer expression");
> +	  TREE_VALUE (TREE_VALUE (attr)) = NULL_TREE;

I'd argue that you want at least when processing_template_decl
store there error_mark_node, to differentiate the cases between
it has been erroneous and already diagnosed vs. it wasn't specified
at all.  For the former, you don't want to error on it in any way
further.  Of course, perhaps for middle-end you want to ensure the
value is INTEGER_CST and valid only, in such a case you'd then
change the error_mark_nodes to e.g. NULL_TREEs or drop the attribute
altogether at !processing_template_decl time only.

> +      allocator = fold_non_dependent_expr (allocator);

I don't understand this, you correctly attempt to fold it into
a constant first.

> +  if (allocator)
> +    {
> +      tree orig_type = TYPE_MAIN_VARIANT (TREE_TYPE (allocator));
> +      if (!INTEGRAL_TYPE_P (TREE_TYPE (allocator))
> +	  || TREE_CODE (orig_type) != ENUMERAL_TYPE
> +	  || TYPE_NAME (orig_type) == NULL_TREE

Here see above comments again.

> +	  || (DECL_NAME (TYPE_NAME (orig_type))
> +	      != get_identifier ("omp_allocator_handle_t")))
> +	{
> +	  error_at (loc2, "%<allocator%> clause expression has type "
> +			  "%qT rather than %<omp_allocator_handle_t%>",
> +			  TREE_TYPE (allocator));
> +	  allocator = TREE_PURPOSE (TREE_VALUE (attr)) = NULL_TREE;
> +	}
> +      else
> +	TREE_PURPOSE (TREE_VALUE (attr)) = fold_non_dependent_expr (allocator);

But then fold it again?  This is useless.

> +    }
> +  if (TREE_STATIC (decl))
> +    {
> +      if (allocator == NULL_TREE)
> +	error_at (loc, "%<allocator%> clause required for "
> +		       "static variable %qD", decl);

See above comments about differentiating something we've diagnosed already
from what you want to diagnose here, missing clause.

> +      else if (allocator
> +	       && (wi::to_widest (allocator) < 1
> +		   || wi::to_widest (allocator) > 8))

And this again should be done only if allocator is not
value_dependent_expression_p, otherwise neither the error nor sorry.

> +	/* 8 = largest predefined memory allocator. */
> +	error_at (loc2, "%<allocator%> clause requires a predefined allocator "
> +			"as %qD is static", decl);
> +      else
> +	sorry_at (loc, "%<#pragma omp allocate%> for static variables like %qD "
> +		       "not yet supported", decl);
> +    }

> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/gomp/allocate-5.C
> @@ -0,0 +1,14 @@
> +template<typename t>
> +t
> +foo()
> +{
> +  t var = 5;
> +  #pragma omp allocate(var) align(sizeof(t) + 1)  /* { dg-error "'align' clause argument needs to be positive constant power of two integer expression" } */

You want to cover in the testsuite all the cases I've described
above, non-template valid and invalid values, template where
the values are not dependent and do fold there to INTEGER_CSTs,
again valid and invalid values, then when the values are value
dependent but have valid/invalid types and finally when they
are type dependent.  And have cases where such templates are
never instantiated (then check for diagnostics that can be done
on such templates or has to be deferred), and have different
templates instantiated with something that is valid and then
others with something that makes it invalid.

Hope this helps.

	Jakub