Am 05.01.24 um 13:23 schrieb Julian Brown:
On Wed, 20 Dec 2023 15:31:15 +0100
Tobias Burnus<tob...@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
