Re: [PATCH v2 06/11] OpenMP: lvalue parsing for map clauses (C++)

[Jakub Jelinek <jakub@redhat.com>]

Hi!

Thanks for working on this!

On Tue, Nov 01, 2022 at 09:50:38PM +0000, Julian Brown wrote:
> > I think we should figure out when we should temporarily disable
> >   parser->omp_array_section_p = false;
> > and restore it afterwards to a saved value.  E.g.
> > cp_parser_lambda_expression seems like a good candidate, the fact that
> > OpenMP array sections are allowed say in map clause doesn't mean they
> > are allowed inside of lambdas and it would be especially hard when
> > the lambda is defining a separate function and the search for
> > OMP_ARRAY_SECTION probably wouldn't be able to discover those.
> > Other spots to consider might be statement expressions, perhaps type
> > definitions etc.
> 
> I've had a go at doing this -- several expression types now forbid
> array-section syntax (see new "bad-array-section-*" tests added). I'm
> afraid my C++ isn't quite up to figuring out how it's possible to
> define a type inside an expression (inside a map clause) if we forbid
> lambdas and statement expressions though -- can you give an example?

But we can't forbid lambdas inside of the map clause expressions,
they are certainly valid in OpenMP, and IMNSHO shouldn't disallow statement
expressions, people might not even know they use a statement expression,
they could just use some standard macro which uses a statement expression
under the hood.  Though your testcases look good.

> > This shouldn't be done just for OMP_CLAUSE_MAP, but for all the
> > other clauses that accept array sections, including
> > OMP_CLAUSE_DEPEND, OMP_CLAUSE_AFFINITY, OMP_CLAUSE_MAP, OMP_CLAUSE_TO,
> > OMP_CLAUSE_FROM, OMP_CLAUSE_INCLUSIVE, OMP_CLAUSE_EXCLUSIVE,
> > OMP_CLAUSE_USE_DEVICE_ADDR, OMP_CLAUSE_HAS_DEVICE_ADDR,
> > OMP_CLAUSE_*REDUCTION.
> 
> I'm not too sure about all of those -- Tobias points out that
> "INCLUSIVE", "EXCLUSIVE", *DEVICE* and *REDUCTION* take "variable list"
> item types, not "locator list", though sometimes with an array section
> being permitted (in OpenMP 5.2+).

That is true.  For the clauses that don't use locator lists but variable
lists but accept array sections there are strict restrictions on what one
can use, basically one can only have varname or varname[...] or
varname[...][...] etc. where ... is the normal array element or array
section syntax.  So, we probably should continue to parse them as now,
but we can use OMP_ARRAY_SECTION to hold what we've parsed or even share
code with parsing array sections and the [...] on those clauses.

> Tested (alongside next patch) with offloading to NVPTX -- with my
> previously-posted "address tokenization" patch also applied.

> 2022-11-01  Julian Brown  <julian@codesourcery.com>
> 
> gcc/c-family/
>         * c-omp.cc (c_omp_address_inspector::map_supported_p): Handle
> 	OMP_ARRAY_SECTION.
> 
> gcc/cp/
> 	* constexpr.cc (potential_consant_expression_1): Handle
> 	OMP_ARRAY_SECTION.
>         * error.cc (dump_expr): Handle OMP_ARRAY_SECTION.
>         * parser.cc (cp_parser_new): Initialize parser->omp_array_section_p.
> 	(cp_parser_statement_expr): Disallow array sections.
>         (cp_parser_postfix_open_square_expression): Support OMP_ARRAY_SECTION
>         parsing.
> 	(cp_parser_parenthesized_expression_list, cp_parser_lambda_expression,
> 	cp_parser_braced_list): Disallow array sections.
>         (cp_parser_omp_var_list_no_open): Remove ALLOW_DEREF parameter, add
>         MAP_LVALUE in its place.  Supported generalised lvalue parsing for
> 	OpenMP map, to and from clauses.
>         (cp_parser_omp_var_list): Remove ALLOW_DEREF parameter, add MAP_LVALUE.
>         Pass to cp_parser_omp_var_list_no_open.
>         (cp_parser_oacc_data_clause, cp_parser_omp_all_clauses): Update calls
>         to cp_parser_omp_var_list.
> 	(cp_parser_omp_clause_map): Add sk_omp scope around
> 	cp_parser_omp_var_list_no_open call.
>         * parser.h (cp_parser): Add omp_array_section_p field.
>         * semantics.cc (handle_omp_array_sections_1): Handle more types of map
>         expression.
>         (handle_omp_array_section): Handle non-DECL_P attachment points.
>         (finish_omp_clauses): Check for supported types of expression.
> 
> gcc/
>         * tree-pretty-print.c (dump_generic_node): Support OMP_ARRAY_SECTION.
>         * tree.def (OMP_ARRAY_SECTION): New tree code.
> 
> gcc/testsuite/
>         * c-c++-common/gomp/map-6.c: Update expected output.
> 	* g++.dg/gomp/bad-array-section-1.C: New test.
> 	* g++.dg/gomp/bad-array-section-2.C: New test.
> 	* g++.dg/gomp/bad-array-section-3.C: New test.
> 	* g++.dg/gomp/bad-array-section-4.C: New test.
> 	* g++.dg/gomp/bad-array-section-5.C: New test.
> 	* g++.dg/gomp/bad-array-section-6.C: New test.
> 	* g++.dg/gomp/bad-array-section-7.C: New test.
> 	* g++.dg/gomp/bad-array-section-8.C: New test.
> 	* g++.dg/gomp/bad-array-section-9.C: New test.
> 	* g++.dg/gomp/has_device_addr-non-lvalue-1.C: New test.
>         * g++.dg/gomp/pr67522.C: Update expected output.
>         * g++.dg/gomp/ind-base-3.C: New test.
>         * g++.dg/gomp/map-assignment-1.C: New test.
>         * g++.dg/gomp/map-inc-1.C: New test.
>         * g++.dg/gomp/map-lvalue-ref-1.C: New test.
>         * g++.dg/gomp/map-ptrmem-1.C: New test.
>         * g++.dg/gomp/map-ptrmem-2.C: New test.
>         * g++.dg/gomp/map-static-cast-lvalue-1.C: New test.
>         * g++.dg/gomp/map-ternary-1.C: New test.
>         * g++.dg/gomp/member-array-2.C: New test.
> 
> libgomp/
> 	* testsuite/libgomp.c++/baseptrs-4.C: Remove commented-out cases that
> 	now work.
>         * testsuite/libgomp.c++/ind-base-1.C: New test.
>         * testsuite/libgomp.c++/ind-base-2.C: New test.
> 	* testsuite/libgomp.c++/lvalue-tofrom-1.C: New test.
> 	* testsuite/libgomp.c++/lvalue-tofrom-2.C: New test.
>         * testsuite/libgomp.c++/map-comma-1.C: New test.
>         * testsuite/libgomp.c++/map-rvalue-ref-1.C: New test.
>         * testsuite/libgomp.c++/member-array-1.C: New test.
>         * testsuite/libgomp.c++/struct-ref-1.C: New test.
>         * testsuite/libgomp.c++/array-field-1.C: New test.
>         * testsuite/libgomp.c++/array-of-struct-1.C: New test.
>         * testsuite/libgomp.c++/array-of-struct-2.C: New test.

Some lines are (correctly) tab indented above, but others 8 spaces.
This will not get through pre-commit hook.

> @@ -5265,6 +5268,9 @@ static cp_expr
>  cp_parser_statement_expr (cp_parser *parser)
>  {
>    cp_token_position start = cp_parser_start_tentative_firewall (parser);
> +  bool saved_omp_array_section_p = parser->omp_array_section_p;
> +
> +  parser->omp_array_section_p = false;

The modern C++ FE way of doing this is
  auto oas = make_temp_override (parser->omp_array_section_p, false);
where you don't need to do anything at the end and it handles say return
in the middle or goto out of the block.  It isn't appropriate when
you need to restore it at some other location than the end of the containing
block.

> +      if ((index && error_operand_p (index))
> +	  || (length && error_operand_p (length)))
> +	return error_mark_node;

error_operand_p (NULL) works and returns false, so you can just do
      if (error_operand_p (index) || error_operand_p (length))
	return error_mark_node;
Though I wonder if the
> +
> +      cp_parser_require (parser, CPP_CLOSE_SQUARE, RT_CLOSE_SQUARE);

shouldn't be done before it, if one of the expressions is erroneous,
but we have there ] after it, I think we want to continue parsing after it.
> +
> +      tree idxtype;
> +
> +      if (index)
> +	index = maybe_constant_value (index);
> +      if (length)
> +	length = maybe_constant_value (length);

This is incorrect in templates (when processing_template_decl).
maybe_constant_value should be only called when !processing_template_decl,
above you should call fold_non_dependent_expr instead.
And those handle (pass through) NULL_TREE, so no need to conditionalize.
Or maybe even better maybe_fold_non_dependent_expr which will only
return different tree from the original if it actually managed to
fold it into a constant.

Another thing is that in the C++ FE we usually do just parsing in
parser.cc and the actual building of the expressions in some routine
in semantics.cc or decl2.cc or so.
The point is that it sometimes needs to be called twice, once
during parsing (where for !processing_template_decl we handle everything
right away, otherwise for processing_template_decl if the expressions
(in your case postfix_expression, index and/or length) are
type_dependent_expression_p, one doesn't do much processing and
just build_min_nt_loc or so some (dependent) tree just to hold
the expressions (OMP_ARRAY_SECTION in your case).
Or, if processing_template_decl but nothing is type dependent, it does some
diagnostics etc. with build_non_dependent_expr around the expressions,
only to throw it away at the end unless it resulted in an error and
still build a tree with the original expressions (but perhaps a
non-dependent type of the whole expression).
And then there needs to be a pt.cc case which handles the raw
OMP_ARRAY_SECTION tree, will recurse on the subexpressions and finally
call the function that is also called during the parsing to build the tree,
but this time usually with !processing_template_decl, so it builds the final
thing.

For the normal array element parsing, this is done in grok_array_decl.

In C++ test coverage, it is usually needed to have normal non-template
tests (those can be often shared with C FE too in c-c++-common/gomp/),
and then some tests in templates, and there both the non-dependent stuff,
say if you write
template <int N>
void foo ()
{
// normal C/C++ subset stuff here
}
void bar () { foo<0> (); }
and then dependent stuff, where you have say
template <typename T, typename U, typename V>
void baz (T x, U y, V z)
{
... map(x[y:z])
}
and the like or say value dependent N from the earlier template
etc. to test that it works properly if some or all expressions are
type or value dependent during parsing and only later on are instantiated.

Yet another thing is that C++23 allows and we handle multidimensional
subscript operator, see
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2128r6.pdf
Right now the multi-dimensional subscript only applies to cases where
there is a user operator[] and I believe OpenMP array sections on the
other side are strictly for cases where overloaded operators do not apply,
but one can certainly write:
  #pragma omp target map(tofrom:a[1,2,3:4:5])
and while in C++20 and older that is parsed as
a[(1,2,3):4:5]
perhaps with a -Wcomma-subscript warning, in C++23 parsing that yields
a vector of expressions.  So, probably we need to diagnose the case
where there is vector of expressions (i.e. 2 or more) as an error.
C++23 also allows a[] but in that case one can't use the OpenMP
array section syntax, it is handled only if [ is immediately followed by ]
so there is no :.

> +
> +      /* If we know the integer bounds, create an index type with exact
> +	 low/high (or zero/length) bounds.  Otherwise, create an incomplete
> +	 array type.  (This mostly only affects diagnostics.)  */
> +      if (index != NULL_TREE
> +	  && length != NULL_TREE
> +	  && TREE_CODE (index) == INTEGER_CST
> +	  && TREE_CODE (length) == INTEGER_CST)
> +	{
> +	  tree low = fold_convert (sizetype, index);
> +	  tree high = fold_convert (sizetype, length);
> +	  high = size_binop (PLUS_EXPR, low, high);
> +	  high = size_binop (MINUS_EXPR, high, size_one_node);
> +	  idxtype = build_range_type (sizetype, low, high);
> +	}
> +      else if ((index == NULL_TREE || integer_zerop (index))
> +	       && length != NULL_TREE
> +	       && TREE_CODE (length) == INTEGER_CST)
> +	idxtype = build_index_type (length);
> +      else
> +	idxtype = NULL_TREE;
> +
> +      tree eltype = ((postfix_expression != error_mark_node
> +		      && TREE_TYPE (postfix_expression))
> +		     ? TREE_TYPE (TREE_TYPE (postfix_expression))
> +		     : NULL_TREE);
> +
> +      tree sectype;
> +
> +      /* It's not an array or pointer type.  Just reuse the type of the
> +	 original expression as the type of the array section (an error will be
> +	 raised anyway, later).  */
> +      if (eltype == NULL_TREE)
> +	sectype = TREE_TYPE (postfix_expression);
> +      else
> +	sectype = build_array_type (eltype, idxtype);
> +
> +      return build3_loc (input_location, OMP_ARRAY_SECTION,
> +			 sectype, postfix_expression, index, length);
> +    }
> +
> +  parser->colon_corrects_to_scope_p = saved_colon_corrects_to_scope_p;
> +
>    /* Look for the closing `]'.  */
>    cp_parser_require (parser, CPP_CLOSE_SQUARE, RT_CLOSE_SQUARE);
>  
> @@ -8484,6 +8563,7 @@ cp_parser_parenthesized_expression_list (cp_parser* parser,
>  {
>    vec<tree, va_gc> *expression_list;
>    bool saved_greater_than_is_operator_p;
> +  bool saved_omp_array_section_p;
>  
>    /* Assume all the expressions will be constant.  */
>    if (non_constant_p)
> @@ -8501,6 +8581,9 @@ cp_parser_parenthesized_expression_list (cp_parser* parser,
>      = parser->greater_than_is_operator_p;
>    parser->greater_than_is_operator_p = true;
>  
> +  saved_omp_array_section_p = parser->omp_array_section_p;
> +  parser->omp_array_section_p = false;

Here the surrounding code uses the old style save/restore, so perhaps
it is ok as is.
> +
>    cp_expr expr (NULL_TREE);
>  
>    /* Consume expressions until there are no more.  */
> @@ -8571,6 +8654,7 @@ cp_parser_parenthesized_expression_list (cp_parser* parser,
>  
>    parser->greater_than_is_operator_p
>      = saved_greater_than_is_operator_p;
> +  parser->omp_array_section_p = saved_omp_array_section_p;
>  
>    return expression_list;
>  }
> @@ -11051,6 +11135,7 @@ cp_parser_lambda_expression (cp_parser* parser)
>      cp_binding_level* implicit_template_scope = parser->implicit_template_scope;
>      bool auto_is_implicit_function_template_parm_p
>          = parser->auto_is_implicit_function_template_parm_p;
> +    bool saved_omp_array_section_p = parser->omp_array_section_p;
>  
>      parser->num_template_parameter_lists = 0;
>      parser->in_statement = 0;
> @@ -11059,6 +11144,7 @@ cp_parser_lambda_expression (cp_parser* parser)
>      parser->implicit_template_parms = 0;
>      parser->implicit_template_scope = 0;
>      parser->auto_is_implicit_function_template_parm_p = false;
> +    parser->omp_array_section_p = false;
>  
>      /* The body of a lambda in a discarded statement is not discarded.  */
>      bool discarded = in_discarded_stmt;
> @@ -11111,6 +11197,7 @@ cp_parser_lambda_expression (cp_parser* parser)
>      parser->implicit_template_scope = implicit_template_scope;
>      parser->auto_is_implicit_function_template_parm_p
>  	= auto_is_implicit_function_template_parm_p;
> +    parser->omp_array_section_p = saved_omp_array_section_p;
>    }

Here too.
>  
>    /* This field is only used during parsing of the lambda.  */
> @@ -25359,6 +25446,9 @@ cp_parser_braced_list (cp_parser* parser, bool* non_constant_p)
>  {
>    tree initializer;
>    location_t start_loc = cp_lexer_peek_token (parser->lexer)->location;
> +  bool saved_omp_array_section_p = parser->omp_array_section_p;
> +
> +  parser->omp_array_section_p = false;

But this could use make_temp_override.

> +      /* This condition doesn't include OMP_CLAUSE_DEPEND or
> +	 OMP_CLAUSE_AFFINITY since lvalue ("locator list") parsing for those is
> +	 handled further down the function.  */
> +      else if (map_lvalue
> +	       && (kind == OMP_CLAUSE_MAP
> +		   || kind == OMP_CLAUSE_TO
> +		   || kind == OMP_CLAUSE_FROM))
> +	{
> +	  auto s = make_temp_override (parser->omp_array_section_p, true);

You use make_temp_override already here ;)

	Jakub