Re: [PATCH 2/8] OpenMP: lvalue parsing for map/to/from clauses (C)

[Tobias Burnus <burnus@net-b.de>]

Julian Brown wrote:
> This patch adds support for parsing general lvalues ("locator list item
> types") for OpenMP "map", "to" and "from" clauses to the C front-end,
> similar to the previously-posted patch for C++.  Such syntax is permitted
> for OpenMP 5.0 and above.  It was previously posted for mainline here

...

In libgomp/libgomp.texi, the following can now be set to 'Y':

@item C/C++'s lvalue expressions in @code{to}, @code{from}
       and @code{map} clauses @tab N @tab

> @@ -11253,16 +11263,41 @@ c_parser_postfix_expression_after_primary (c_parser *parser,
>   	case CPP_OPEN_SQUARE:
>   	  /* Array reference.  */
>   	  c_parser_consume_token (parser);
> -	  idx = c_parser_expression (parser).value;
> -	  c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE,
> -				     "expected %<]%>");
> -	  start = expr.get_start ();
> -	  finish = parser->tokens_buf[0].location;
> -	  expr.value = build_array_ref (op_loc, expr.value, idx);
> -	  set_c_expr_source_range (&expr, start, finish);
> -	  expr.original_code = ERROR_MARK;
> -	  expr.original_type = NULL;
> -	  expr.m_decimal = 0;
> +	  idx = len = NULL_TREE;
> +	  if (!c_omp_array_section_p
> +	      || c_parser_next_token_is_not (parser, CPP_COLON))
> +	    idx = c_parser_expression (parser).value;
> +
> +	  if (c_omp_array_section_p
> +	      && c_parser_next_token_is (parser, CPP_COLON))
> +	    {
> +	      c_parser_consume_token (parser);
> +	      if (c_parser_next_token_is_not (parser, CPP_CLOSE_SQUARE))
> +		len = c_parser_expression (parser).value;
> +
> +	      c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE,
> +					 "expected %<]%>");
> +
> +	      start = expr.get_start ();
> +	      finish = parser->tokens_buf[0].location;
> +	      expr.value = build_omp_array_section (op_loc, expr.value, idx,
> +						    len);
> +	      set_c_expr_source_range (&expr, start, finish);
> +	      expr.original_code = ERROR_MARK;
> +	      expr.original_type = NULL;
> +	    }
> +	  else
> +	    {
> +	      c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE,
> +					 "expected %<]%>");
> +	      start = expr.get_start ();
> +	      finish = parser->tokens_buf[0].location;
> +	      expr.value = build_array_ref (op_loc, expr.value, idx);
> +	      set_c_expr_source_range (&expr, start, finish);
> +	      expr.original_code = ERROR_MARK;
> +	      expr.original_type = NULL;
> +	      expr.m_decimal = 0;
> +	    }

I think that's more readable when moving everything but the expr.value 
assignment after the if/else (obviously for "if" also everything until 
"len =" has to remain). That also adds the missing "m_decimal = 0" for 
the "if" case.

> @@ -13915,8 +13955,97 @@ c_parser_omp_variable_list (c_parser *parser,
...
> +	  else if (TREE_CODE (decl) == INDIRECT_REF)
> +	    {
> +	      /* Turn *foo into the representation previously used for
> +		 foo[0].  */
> +	      decl = TREE_OPERAND (decl, 0);
> +	      STRIP_NOPS (decl);
> +
> +	      decl = build_omp_array_section (loc, decl, integer_zero_node,
> +					      integer_one_node);
> +	    }

I wonder whether we shouldn't use the C++ wording, i.e.

               /* If we have "*foo" and
                  - it's an indirection of a reference, "unconvert" it,
                    i.e. strip the indirection (to just "foo").
                  - it's an indirection of a pointer, turn it into
                    "foo[0:1]".  */

* * *

As remarked for cp/typecheck.cc's build_omp_array_section:

> +tree
> +build_omp_array_section (location_t loc, tree array, tree index, tree length)
> +{
> +  tree idxtype;
> +
> +  if (index != NULL_TREE
> +      && length != NULL_TREE
> +      && INTEGRAL_TYPE_P (TREE_TYPE (index))
> +      && INTEGRAL_TYPE_P (TREE_TYPE (length)))
> +    {
> +      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
> +	   && INTEGRAL_TYPE_P (TREE_TYPE (length)))
> +    idxtype = build_index_type (length);
> +  else
> +    idxtype = NULL_TREE;
> +
> +  tree type = TREE_TYPE (array);
> +  gcc_assert (type);
> +
> +  tree sectype, eltype = TREE_TYPE (type);
> +
> +  /* 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
> +      || error_operand_p (eltype)
> +      || error_operand_p (idxtype))
> +    sectype = TREE_TYPE (array);
> +  else
> +    sectype = build_array_type (eltype, idxtype);
> +
> +  return build3_loc (loc, OMP_ARRAY_SECTION, sectype, array, index, length);
> +}

I think that's more readable when having that if condition at the top
and moving everything before into the else branch.


Otherwise, LGTM.

Thanks for the patch!

Tobias