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

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

Am 05.01.24 um 13:23 schrieb Julian Brown:
> On Wed, 20 Dec 2023 15:31:15 +0100
> Tobias Burnus<tobias@codesourcery.com>  wrote:
> Here's a rebased/retested version which fixes those bits (I haven't
> adjusted the libgomp.texi bit you noted yet, though).
> 
> How does this look now?


> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -13499,7 +13499,11 @@ gimplify_adjust_omp_clauses_1 (splay_tree_node n, void *data)
>        if (TREE_CODE (dtype) == REFERENCE_TYPE)
>         dtype = TREE_TYPE (dtype);
>        /* FIRSTPRIVATE_POINTER doesn't work well if we have a
> -        multiply-indirected pointer.  */
> +        multiply-indirected pointer.  If we have a reference to a pointer to
> +        a pointer, it's possible that this should really be
> +        GOMP_MAP_FIRSTPRIVATE_REFERENCE -- but that also doesn't work at the
> +        moment, so stick with this.  (See testcase
> +        baseptrs-4.C:ref2ptrptr_offset_decl_member_slice).  */

Looks as we should have a tracking PR about this; can you file one?

* * *

> +  if (processing_template_decl)
> +    {
> +      if (type_dependent_expression_p (array_expr)
> +         || type_dependent_expression_p (index)
> +         || type_dependent_expression_p (length))
> +       return build_min_nt_loc (loc, OMP_ARRAY_SECTION, array_expr, index,
> +                                length);
> +    }

I personally find it more readable if combined in a single 'if' condition.

> +             /* Turn *foo into foo[0:1].  */
> +             decl = TREE_OPERAND (decl, 0);
> +             STRIP_NOPS (decl);
> +
> +             /* 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]".  */
> +             if (!ref_p)
> +               decl = grok_omp_array_section (loc, decl, integer_zero_node,
> +                                              integer_one_node);

I would remove the first comment and remove the two succeeding lines 
below the second comment.


> +         /* This code rewrites a parsed expression containing various tree
> +            codes used to represent array accesses into a more uniform nest of
> +            OMP_ARRAY_SECTION nodes before it is processed by
> +            semantics.cc:handle_omp_array_sections_1.  It might be more
> +            efficient to move this logic to that function instead, analysing
> +            the parsed expression directly rather than this preprocessed
> +            form.  */

Or to do this transformation in handle_omp_array_sections to get still a 
unified result in the middle end. I see advantages of all three 
solutions. (Doing this in parse.cc (as currently done) feels a bit odd, 
though.)

* * *

> build_omp_array_section (location_t loc, tree array_expr, tree index,
> +			 tree length)
> +{
> +  tree idxtype;
> +
> +  /* 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 type = TREE_TYPE (array_expr);
> +  gcc_assert (type);
> +  type = non_reference (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)
> +    sectype = TREE_TYPE (array_expr);
> +  else
> +    sectype = build_array_type (eltype, idxtype);
> +
> +  return build3_loc (loc, OMP_ARRAY_SECTION, sectype, array_expr, index,
> +		     length);
> +}

I wonder whether it would be more readable if one moves all the 
'idxtype' handling into the last 'else' branch.

* * *

LGTM - please file the PR and consider the readability items above.

Thanks,

Tobias