On Mon, Nov 20, 2023 at 11:42:02AM +0100, Tobias Burnus wrote:
> 2023-11-19 Tobias Burnus <tob...@codesourcery.com>
> Chung-Lin Tang <clt...@codesourcery.com>
>
> gcc/ChangeLog:
>
> * builtin-types.def (BT_FN_VOID_PTRMODE):
> (BT_FN_PTRMODE_PTRMODE_INT_PTR): Add.
> * gimplify.cc (gimplify_bind_expr): Diagnose missing
> uses_allocators clause.
> (gimplify_scan_omp_clauses, gimplify_adjust_omp_clauses,
> gimplify_omp_workshare): Handle uses_allocators.
> * omp-builtins.def (BUILT_IN_OMP_INIT_ALLOCATOR,
> BUILT_IN_OMP_DESTROY_ALLOCATOR): Add.
> * omp-low.cc (scan_sharing_clauses):
Missing description.
> +static tree
> +c_parser_omp_clause_uses_allocators (c_parser *parser, tree list)
> +{
> + location_t clause_loc = c_parser_peek_token (parser)->location;
> + tree t = NULL_TREE, nl = list;
> + matching_parens parens;
> + if (!parens.require_open (parser))
> + return list;
> +
> + tree memspace_expr = NULL_TREE;
> + tree traits_var = NULL_TREE;
> +
> + struct item_tok
> + {
> + location_t loc;
> + tree id;
> + item_tok (void) : loc (UNKNOWN_LOCATION), id (NULL_TREE) {}
> + };
> + struct item { item_tok name, arg; };
> + auto_vec<item> *modifiers = NULL, *allocators = NULL;
> + auto_vec<item> *cur_list = new auto_vec<item> (4);
This is certainly the first time I've seen pointers to auto_vec,
normally one uses just vec in such cases, auto_vec is used typically
on automatic variables to make sure the destruction is done.
But I think all the first parse it as a token soup without checking
anything and only in the second round actually check it is something
we've never done before in exactly the same situations.
The usual way would be to quickly peek at tokens to see if there is
: ahead and decide based on that.
See e.g. c_parser_omp_clause_allocate clause.
That has_modifiers check could be basically copied over with
the names of modifiers changed, or could be done in a loop, or
could be moved into a helper function which could be used
by c_parser_omp_clause_allocate and this function and perhaps
others, pass it the list of modifiers and have it return whether
there are modifiers or not.
c_parser_omp_clause_linear does this too (though, that has one of
the modifiers without arguments).
I'm afraid if parsing of every clause does the parsing so significantly
differently it will be a maintainance nightmare.
> @@ -23648,7 +23861,8 @@ c_parser_omp_target_exit_data (location_t loc,
> c_parser *parser,
> | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_IN_REDUCTION) \
> | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_THREAD_LIMIT) \
> | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_IS_DEVICE_PTR)\
> - | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_HAS_DEVICE_ADDR))
> + | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_HAS_DEVICE_ADDR)\
Space before \ please (and also in the line above.
> + if (strcmp (IDENTIFIER_POINTER (DECL_NAME (t)),
> + "omp_null_allocator") == 0)
> + {
> + error_at (OMP_CLAUSE_LOCATION (c),
> + "%<omp_null_allocator%> cannot be used in "
> + "%<uses_allocators%> clause");
> + break;
> + }
> +
> + if (OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE (c)
> + || OMP_CLAUSE_USES_ALLOCATORS_TRAITS (c))
> + {
> + error_at (OMP_CLAUSE_LOCATION (c),
> + "modifiers cannot be used with pre-defined "
> + "allocators");
> + break;
> + }
> + }
> + t = OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE (c);
> + if (t != NULL_TREE
> + && (TREE_CODE (t) != CONST_DECL
> + || TREE_CODE (TREE_TYPE (t)) != ENUMERAL_TYPE
> + || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (TREE_TYPE
> (t))),
> + "omp_memspace_handle_t") != 0))
> + {
> + error_at (OMP_CLAUSE_LOCATION (c), "memspace modifier must be "
Maybe %<memspace%> ?
> + "constant enum of %<omp_memspace_handle_t%> type");
> + remove = true;
> + break;
> + }
> + t = OMP_CLAUSE_USES_ALLOCATORS_TRAITS (c);
> + if (t != NULL_TREE)
> + {
> + bool type_err = false;
> +
> + if (TREE_CODE (TREE_TYPE (t)) != ARRAY_TYPE
> + || DECL_SIZE (t) == NULL_TREE)
> + type_err = true;
> + else
> + {
> + tree elem_t = TREE_TYPE (TREE_TYPE (t));
> + if (TREE_CODE (elem_t) != RECORD_TYPE
> + || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (elem_t)),
> + "omp_alloctrait_t") != 0
> + || !TYPE_READONLY (elem_t))
> + type_err = true;
> + }
> + if (type_err)
> + {
> + if (TREE_CODE (t) != ERROR_MARK)
t != error_mark_node
> + error_at (OMP_CLAUSE_LOCATION (c), "traits array %qE must "
> + "be of %<const omp_alloctrait_t []%> type", t);
> + else
> + error_at (OMP_CLAUSE_LOCATION (c), "traits array must "
> + "be of %<const omp_alloctrait_t []%> type");
> + remove = true;
> + tree cst_val = decl_constant_value_1 (t, true);
> + if (cst_val == t)
> + {
> + error_at (OMP_CLAUSE_LOCATION (c), "traits array must be "
> + "of constant values");
must be initialized with constants?
> + struct item_tok
> + {
> + location_t loc;
> + tree id;
> + item_tok (void) : loc (UNKNOWN_LOCATION), id (NULL_TREE) {}
> + };
> + struct item { item_tok name, arg; };
> + auto_vec<item> *modifiers = NULL, *allocators = NULL;
> + auto_vec<item> *cur_list = new auto_vec<item> (4);
Same as what I wrote in C.
> + case OMP_CLAUSE_USES_ALLOCATORS:
> + t = OMP_CLAUSE_USES_ALLOCATORS_ALLOCATOR (c);
> + if (TREE_CODE (t) == FIELD_DECL)
> + {
> + sorry_at (OMP_CLAUSE_LOCATION (c), "class members not yet "
> + "supported in %<uses_allocators%> clause");
> + remove = true;
> + break;
> + }
> + t = convert_from_reference (t);
> + if (TREE_CODE (TREE_TYPE (t)) != ENUMERAL_TYPE
> + || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (TREE_TYPE (t))),
> + "omp_allocator_handle_t") != 0)
Back to what I wrote about the C++ allocators, if processing_template_decl t
can be type or value dependent and I think we shouldn't stay in the way to
it.
Type dependent as in:
template <typename T, T X>
void foo (T x)
{
#pragma omp target uses_allocators (memspace (...), traits (...): x)
;
#pragma omp target uses_allocators (X)
;
}
instantiated with foo <omp_allocator_handle_t, omp_low_lat_mem_alloc> (...)
or so. So the usual drill, if it is type dependent, defer checking the
type for later. If it is value dependent, don't check exact value and
defer for later.
The above also brings the admittedly preexisting issue, do we want to
support the case where the allocator has type oaht from
typedef omp_allocator_handle_t oaht;
or not? I guess the wording in the standard is unclear on that, users
would certainly expect typedefs/using to work as well (so then
it should be TYPE_MAIN_VARIANT), but perhaps reading OpenMP pedantically
expression of type XYZ is just that type and nothing else.
> + {
> + error_at (OMP_CLAUSE_LOCATION (c),
> + "allocator must be of %<omp_allocator_handle_t%> type");
> + remove = true;
> + break;
> + }
> + if (TREE_CODE (t) == CONST_DECL)
This is another question (also for C, at least C23).
While the allocator needs to be an identifier, so the enumerator or some
variable, not some random other expressions (so e.g. calls to
constexpr/consteval functions are not ok), I wonder whether
constexpr omp_allocator_handle_t myh = omp_low_lat_mem_alloc;
... uses_allocator (myh) ...
is ok or not and ditto the above mentioned uses_allocator (X).
I bet in such case one might not get a CONST_DECL here, and instead if
not value dependent should fold_non_dependent_expr and check if it yields
an INTEGER_CST.
> + {
> + /* Currently for pre-defined allocators in libgomp, we do not
> + require additional init/fini inside target regions, so discard
> + such clauses. */
> + remove = true;
> +
> + if (strcmp (IDENTIFIER_POINTER (DECL_NAME (t)),
> + "omp_null_allocator") == 0)
For omp_null_allocator it would then be integer_zerop check.
> + {
> + error_at (OMP_CLAUSE_LOCATION (c),
> + "%<omp_null_allocator%> cannot be used in "
> + "%<uses_allocators%> clause");
> + break;
> + }
> +
> + if (OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE (c)
> + || OMP_CLAUSE_USES_ALLOCATORS_TRAITS (c))
> + {
> + error_at (OMP_CLAUSE_LOCATION (c),
> + "modifiers cannot be used with pre-defined "
> + "allocators");
> + break;
> + }
> + }
> + t = OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE (c);
> + if (t != NULL_TREE
> + && (TREE_CODE (t) != CONST_DECL
> + || TREE_CODE (TREE_TYPE (t)) != ENUMERAL_TYPE
> + || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (TREE_TYPE
> (t))),
> + "omp_memspace_handle_t") != 0))
> + {
> + error_at (OMP_CLAUSE_LOCATION (c), "memspace modifier must be "
> + "constant enum of %<omp_memspace_handle_t%> type");
I find this OpenMP standard wording just weird but perhaps it is just
because I'm not a native English speaker. I'd write with instead of of.
> + if (TREE_CODE (TREE_TYPE (t)) != ARRAY_TYPE
> + || DECL_SIZE (t) == NULL_TREE)
|| !COMPLETE_TYPE_P (t)
?
> + type_err = true;
> + else
> + {
> + tree elem_t = TREE_TYPE (TREE_TYPE (t));
> + if (TREE_CODE (elem_t) != RECORD_TYPE
> + || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (elem_t)),
> + "omp_alloctrait_t") != 0
Again, question is about typedefs.
> + for (tree *cp = &OMP_CLAUSES (expr); *cp != NULL;
> + cp = &OMP_CLAUSE_CHAIN (*cp))
> + if (OMP_CLAUSE_CODE (*cp) == OMP_CLAUSE_USES_ALLOCATORS)
> + {
> + tree c = *cp;
> + tree allocator = OMP_CLAUSE_USES_ALLOCATORS_ALLOCATOR (c);
> + tree memspace = OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE (c);
> + tree traits = OMP_CLAUSE_USES_ALLOCATORS_TRAITS (c);
> +
> + if (omp_init_allocator_fn == NULL_TREE)
> + {
> + omp_init_allocator_fn
> + = builtin_decl_explicit (BUILT_IN_OMP_INIT_ALLOCATOR);
> + omp_destroy_allocator_fn
> + = builtin_decl_explicit (BUILT_IN_OMP_DESTROY_ALLOCATOR);
> + }
What is unclear to me is how this is supposed to behave in target teams
case. Whether it is acceptable that each team calls omp_init_allocator
and each team has a separate value for the privatized var, or if we'd need
to arrange for it to happen just once and be propagated to other teams
somehow (but that is perhaps impossible).
> + tree ntraits, traits_var;
> + if (traits == NULL_TREE)
> + {
> + ntraits = integer_zero_node;
> + traits_var = null_pointer_node;
> + }
> + else if (DECL_INITIAL (traits))
> + {
> + location_t loc = OMP_CLAUSE_LOCATION (c);
> + tree t = DECL_INITIAL (traits);
> + gcc_assert (TREE_CODE (t) == CONSTRUCTOR);
> + ntraits = build_int_cst (integer_type_node,
> + CONSTRUCTOR_NELTS (t));
We certainly shouldn't derive ntraits from whether it is a CONSTRUCTOR
and how many CONSTRUCTOR_NELTS it has, it could have fewer than the
array size. IMHO we should go with the array_type_nelts in both cases
(and complain perhaps already in the FEs if it is not constant, we don't
want VLAs there I suppose). Perhaps repeat the CONSTRUCTOR check
for DECL_INITIAL as well earlier when you decide if it should be
firstprivatized or not.
> + t = get_initialized_tmp_var (t, &init_seq, NULL);
> + traits_var = build_fold_addr_expr_loc (loc, t);
> + }
> + else
> + {
> + location_t loc = OMP_CLAUSE_LOCATION (c);
> + gcc_assert (TREE_CODE (TREE_TYPE (traits)) == ARRAY_TYPE);
> + tree t = TYPE_DOMAIN (TREE_TYPE (traits));
> + tree min = TYPE_MIN_VALUE (t);
> + tree max = TYPE_MAX_VALUE (t);
> + gcc_assert (TREE_CODE (min) == INTEGER_CST
> + && TREE_CODE (max) == INTEGER_CST);
> + t = fold_build2_loc (loc, MINUS_EXPR, TREE_TYPE (min),
> + max, min);
> + t = fold_build2_loc (loc, PLUS_EXPR, TREE_TYPE (min),
> + t, build_int_cst (TREE_TYPE (min), 1));
> + ntraits = t;
Use array_type_nelts.
Jakub
