Re: [PATCH, OpenMP, v2] Implement uses_allocators clause for target regions
On Mon, Jun 06, 2022 at 09:38:30PM +0800, Chung-Lin Tang wrote: > I'll file a bugzilla for the target construct. Thanks. > That said, can we delay FIELD_DECL support for uses_allocators? (which is > target construct only) > Since it appears to be not trivial at the moment. Sure. But would be nice to file a PR to track it once the patch is committed. Jakub
Re: [PATCH, OpenMP, v2] Implement uses_allocators clause for target regions
On 2022/6/6 9:22 下午, Jakub Jelinek wrote:
On Mon, Jun 06, 2022 at 09:19:18PM +0800, Chung-Lin Tang wrote:
On 2022/5/31 6:02 PM, Jakub Jelinek wrote:
5) for C++, we should handle FIELD_DECLs, but it shouldn't be hard, just
look how it is handled for private too
Jakub
About private() for non-static members, is it really working right now?
Perhaps we have a bug that we should file in bugzilla and should fix.
Can you try omp parallel or omp target in the test instead?
I see it works for omp parallel/task, gimplify results:
void C::foo (struct C * const this)
{
omp_allocator_handle_t a [value-expr: ((struct C *) this)->a];
#pragma omp parallel private(a)
{
a = 0;
}
}
I'll file a bugzilla for the target construct.
That said, can we delay FIELD_DECL support for uses_allocators? (which is
target construct only)
Since it appears to be not trivial at the moment.
Thanks,
Chung-Lin
A simple test:
struct C {
omp_allocator_handle_t a;
void foo (void) {
#pragma omp target private (a)
a = (omp_allocator_handle_t) 0;
}
};
int main (void)
{
C c;
c.foo ();
return 0;
}
Jakub
Re: [PATCH, OpenMP, v2] Implement uses_allocators clause for target regions
On Mon, Jun 06, 2022 at 09:19:18PM +0800, Chung-Lin Tang wrote:
> On 2022/5/31 6:02 PM, Jakub Jelinek wrote:
> > 5) for C++, we should handle FIELD_DECLs, but it shouldn't be hard, just
> > look how it is handled for private too
> >
> > Jakub
>
> About private() for non-static members, is it really working right now?
Perhaps we have a bug that we should file in bugzilla and should fix.
Can you try omp parallel or omp target in the test instead?
> A simple test:
>
> struct C {
> omp_allocator_handle_t a;
> void foo (void) {
> #pragma omp target private (a)
> a = (omp_allocator_handle_t) 0;
> }
> };
>
> int main (void)
> {
> C c;
> c.foo ();
> return 0;
> }
Jakub
Re: [PATCH, OpenMP, v2] Implement uses_allocators clause for target regions
On 2022/5/31 6:02 PM, Jakub Jelinek wrote:
5) for C++, we should handle FIELD_DECLs, but it shouldn't be hard, just
look how it is handled for private too
Jakub
About private() for non-static members, is it really working right now?
A simple test:
struct C {
omp_allocator_handle_t a;
void foo (void) {
#pragma omp target private (a)
a = (omp_allocator_handle_t) 0;
}
};
int main (void)
{
C c;
c.foo ();
return 0;
}
After C++ front-end processing we get:
{
omp_allocator_handle_t D.2823 [value-expr: ((struct C *) this)->a];
#pragma omp target private(D.2823)
{
{
<;
}
}
}
The OMP field privatization seems to be doing something here.
However gimplify turns this into:
void C::foo (struct C * const this)
{
omp_allocator_handle_t a [value-expr: ((struct C *) this)->a];
#pragma omp target num_teams(1) thread_limit(0) private(a) \
map(alloc:MEM[(char *)this] [len: 0]) map(firstprivate:this [pointer
assign, bias: 0])
{
this->a = 0;
}
}
This doesn't look quite right for private clause. I don't quite expect a
zero-length mapping of this[:0],
nor reverting the gimple to use "this->a" for a private copy.
Chung-Lin
Re: [PATCH, OpenMP, v2] Implement uses_allocators clause for target regions
On Mon, May 30, 2022 at 07:23:55PM +0200, Jakub Jelinek via Gcc-patches wrote: > On Mon, May 30, 2022 at 10:43:30PM +0800, Chung-Lin Tang wrote: > > > This feels like you only accept a single allocator in the new syntax, > > > but that isn't my reading of the spec, I'd understand it as: > > > uses_allocators (memspace(omp_high_bw_mem_space), traits(foo_traits) : > > > bar, baz, qux) > > > being valid too. > > > > This patch now allows multiple allocators to be specified in new syntax, > > although I have > > to note that the 5.2 specification of uses_allocators (page 181) > > specifically says > > "allocator: expression of allocator_handle_type" for the "Arguments" > > description, > > not a "list" like the allocate clause. > > I guess this should be raised on omp-lang then what we really want. > Because the 5.1 syntax definitely allowed multiple allocators. The response I got on omp-lang is that it is intentional that in the new syntax only a single allocator is allowed. So I'd suggest to implement: 1) if has_modifiers (i.e. certainly new syntax), only allow a single enumerator / identifier for a variable and no ()s after it 2) if !has_modifiers and there is exactly one allocator without ()s, treat it like new syntax 3) otherwise, it is the old (5.1) syntax, which allows a list and that list can contain ()s for traits, but in the light of the 5.2 wording, I'd even for that case avoid diagnosing missing traits for non-predefined allocators 4) omp_null_allocator should be diagnosed as invalid, private (omp_null_allocator) is rejected... 5) for C++, we should handle FIELD_DECLs, but it shouldn't be hard, just look how it is handled for private too Jakub
Re: [PATCH, OpenMP, v2] Implement uses_allocators clause for target regions
On Mon, May 30, 2022 at 10:43:30PM +0800, Chung-Lin Tang wrote:
> > This feels like you only accept a single allocator in the new syntax,
> > but that isn't my reading of the spec, I'd understand it as:
> > uses_allocators (memspace(omp_high_bw_mem_space), traits(foo_traits) : bar,
> > baz, qux)
> > being valid too.
>
> This patch now allows multiple allocators to be specified in new syntax,
> although I have
> to note that the 5.2 specification of uses_allocators (page 181) specifically
> says
> "allocator: expression of allocator_handle_type" for the "Arguments"
> description,
> not a "list" like the allocate clause.
I guess this should be raised on omp-lang then what we really want.
Because the 5.1 syntax definitely allowed multiple allocators.
> > It should be only removed if we emit the error (again with break; too).
> > IMHO (see the other mail) we should complain here if it has value 0
> > (the omp_null_allocator case), dunno if we should error or just warn
> > if the value is outside of the range of known predefined identifiers (1..8
> > currently I think). > But, otherwise, IMHO we need to keep it around,
> > perhaps replacing the
> > CONST_DECL with INTEGER_CST, for the purposes of checking what predefined
> > allocators are used in the region.
>
> omp_alloc in libgomp does handle the omp_null_allocator case, by converting it
> to something else.
Sure, but the spec says that omp_alloc (42, omp_null_allocator) is invalid
in target regions unless requires dynamic_allocators is seen first.
And uses_allocators (omp_null_allocator) shouldn't make that valid.
> @@ -15651,6 +15653,199 @@ c_parser_omp_clause_allocate (c_parser *parser,
> tree list)
>return nl;
> }
>
> +/* OpenMP 5.0:
> + uses_allocators ( allocator-list )
> +
> + allocator-list:
> + allocator
> + allocator , allocator-list
> + allocator ( traits-array )
> + allocator ( traits-array ) , allocator-list
> +
> + OpenMP 5.2:
> +
> + uses_allocators ( modifier : allocator-list )
> + uses_allocators ( modifier , modifier : allocator-list )
> +
> + modifier:
> + traits ( traits-array )
> + memspace ( mem-space-handle ) */
> +
> +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 *modifiers = NULL, *allocators = NULL;
> + auto_vec *cur_list = new auto_vec (4);
Each vec/auto_vec with a new type brings quite some overhead,
a lot of functions need to be instantiated for it.
I think it would be far easier to use raw token parsing:
unsigned int pos = 1;
bool has_modifiers = false;
while (true)
{
c_token *tok = c_parser_peek_nth_token_raw (parser, pos);
if (tok->type != CPP_NAME)
break;
++pos;
if (c_parser_peek_nth_token_raw (parser, pos + 1)->type
== CPP_OPEN_PAREN)
{
++pos;
if (!c_parser_check_balanced_raw_token_sequence (parser, )
|| c_parser_peek_nth_token_raw (parser, pos)->type
!= CPP_CLOSE_PAREN)
break;
++pos;
}
tok = c_parser_peek_nth_token_raw (parser, pos);
if (tok->type == CPP_COLON)
{
has_modifiers = true;
break;
}
if (tok->type != CPP_COMMA)
break;
++pos;
}
This should (haven't tested it though, so sorry if there are errors)
cheaply determine if one should or shouldn't parse modifiers and
then can just do parsing of modifiers if (has_modifiers) and
then just the list (with ()s inside of list only if (!has_modifiers)).
> @@ -21093,7 +21292,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)\
Can you please fix up both the IS_DEVICE_PTR and newly added
HAS_DEVICE_ADDR change to have a space before \ ?
> + | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_USES_ALLOCATORS))
> +static tree
> +cp_parser_omp_clause_uses_allocators (cp_parser *parser, tree list)
> +{
> + location_t clause_loc
> += cp_lexer_peek_token (parser->lexer)->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;
> +
>
Re: [PATCH, OpenMP, v2] Implement uses_allocators clause for target regions
Hi Jakub,
this is v3 of the uses_allocators patch.
On 2022/5/20 1:46 AM, Jakub Jelinek wrote:
On Tue, May 10, 2022 at 07:29:23PM +0800, Chung-Lin Tang wrote:
@@ -15624,6 +15626,233 @@ c_parser_omp_clause_allocate (c_parser *parser, tree
list)
return nl;
}
+/* OpenMP 5.2:
+ uses_allocators ( allocator-list )
As uses_allocators is a 5.0 feature already, the above should say
/* OpenMP 5.0:
+
+ allocator-list:
+ allocator
+ allocator , allocator-list
+ allocator ( traits-array )
+ allocator ( traits-array ) , allocator-list
+
And here it should add
OpenMP 5.2:
Done.
+ if (c_parser_next_token_is (parser, CPP_NAME))
+{
+ c_token *tok = c_parser_peek_token (parser);
+ const char *p = IDENTIFIER_POINTER (tok->value);
+
+ if (strcmp ("traits", p) == 0 || strcmp ("memspace", p) == 0)
+ {
+ has_modifiers = true;
+ c_parser_consume_token (parser);
+ matching_parens parens2;;
Double ;;, should be just ;
But more importantly, it is more complex.
When you see
uses_allocators(traits or
uses_allocators(memspace
it is not given that it has modifiers. While the 5.0/5.1 syntax had
a restriction that when allocator is not a predefined allocator (and
traits or memspace aren't predefined allocators) it must use ()s with
traits, so
uses_allocators(traits)
uses_allocators(memspace)
uses_allocators(traits,memspace)
are all invalid,
omp_allocator_handle_t traits;
uses_allocators(traits(mytraits))
or
omp_allocator_handle_t memspace;
uses_allocators(memspace(mytraits),omp_default_mem_alloc)
are valid in the old syntax.
So, I'm afraid to find out if the traits or memspace identifier
seen after uses_allocator ( are modifiers or not we need to
peek (for C with c_parser_peek_nth_token_raw) through all the
modifiers whether we see a : and only in that case say they
are modifiers rather than the old style syntax.
The parser parts have been rewritten to allow this kind of use now.
New code essentially parses lists of "id(id), id(id), ...", possibly delimited
by a ':' marking the modifier/allocator lists.
I don't really like the modifiers handling not done in a loop.
As I said above, there needs to be some check whether there are modifiers or
not, but once we figure out there are modifiers, it should be done in a loop
with say some mask var on which traits have been already handled to diagnose
duplicates, we don't want to do the parsing code twice.
Now everything is done in loops. The new code should be considerably simpler
now.
This feels like you only accept a single allocator in the new syntax,
but that isn't my reading of the spec, I'd understand it as:
uses_allocators (memspace(omp_high_bw_mem_space), traits(foo_traits) : bar,
baz, qux)
being valid too.
This patch now allows multiple allocators to be specified in new syntax,
although I have
to note that the 5.2 specification of uses_allocators (page 181) specifically
says
"allocator: expression of allocator_handle_type" for the "Arguments"
description,
not a "list" like the allocate clause.
+ case OMP_CLAUSE_USES_ALLOCATORS:
+ t = OMP_CLAUSE_USES_ALLOCATORS_ALLOCATOR (c);
+ if (bitmap_bit_p (_head, DECL_UID (t))
+ || bitmap_bit_p (_head, DECL_UID (t))
+ || bitmap_bit_p (_head, DECL_UID (t))
+ || bitmap_bit_p (_head, DECL_UID (t)))
You can't just use DECL_UID before you actually verify it is a variable.
So IMHO this particular if should be moved down somewhat.
Guarded now.
+ {
+ error_at (OMP_CLAUSE_LOCATION (c),
+ "%qE appears more than once in data clauses", t);
+ remove = true;
+ }
+ else
+ bitmap_set_bit (_head, DECL_UID (t));
+ if (TREE_CODE (TREE_TYPE (t)) != ENUMERAL_TYPE
+ || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (TREE_TYPE (t))),
+"omp_allocator_handle_t") != 0)
+ {
+ error_at (OMP_CLAUSE_LOCATION (c),
+ "allocator must be of % type");
+ remove = true;
+ }
I'd add break; after remove = true;
Added some such breaks.
+ if (TREE_CODE (t) == CONST_DECL)
+ {
+ 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");
+
+ /* Currently for pre-defined allocators in libgomp, we do not
+require additional init/fini inside target regions, so discard
+such clauses. */
+ remove = true;
+ }
It should be only removed if we emit the error (again with break; too).
IMHO (see the other mail) we should complain here if it has value 0
(the omp_null_allocator case), dunno if we should error or just warn
if the value is
Re: [PATCH, OpenMP, v2] Implement uses_allocators clause for target regions
Hi Jakub,
On 19.05.22 18:00, Jakub Jelinek wrote:
On Tue, May 10, 2022 at 07:29:23PM +0800, Chung-Lin Tang wrote:
I've attached v2 of the patch. Currently in testing.
Just a general rant, the non-requires dynamic_allocators support
seems to be a total mess in the standard. Probably something
that should be discussed on omp-lang.
Or in some issue. Some newer developments (all links unfortunately
nonpublic):
There is now a nearly ready example for the 5.2.1 example document, cf.
https://github.com/OpenMP/examples-internal/issues/275
https://github.com/OpenMP/spec/issues/3229 improves the wording related
to 'requires dynamic_allocators' – vote was after the 5.2 release.
[...]Allocators can appear in various places, with requires dynamic_allocators
it is all clear, the only allocators required to be constant expressions
(aka predefined allocators) are on allocate for non-automatic variables
Side remark: the OMP_ALLOCATOR environment variable sets the
def-allocator-var ICV – but besides pre-defined allocators, it also
permits to define (traits, memspace, ...) a new default allocator (new in OMP
5.1).
And this one can then seemingly also be used in the target region
(only with 'requires dynamic_allocators').
I note that GCC supports OMP_ALLOCATOR but seemingly only with
predefined allocators (→ OMP 5.0). – I think we need to open PR for that one
and/or a new line in the 5.1 implementation tables.
(my understanding is that omp_null_allocator isn't valid for those but I
could be wrong),
(I read it likewise as it is not predefined,)
Without requires dynamic_allocators, there are various extra restrictions
imposed:
1) omp_init_allocator/omp_destroy_allocator [...]
2) omp_alloc etc. [...]
3) for allocate directive on static vars [...]
4) for allocate clause e.g. when privatizing stuff or allocate directive
for automatic vars no such restrictions exist
Now, that means that e.g. the user provided uses_allocators without
requires dynamic_allocators are only useful for the case 4), it is unclear
if that was really intended.
Note that (4) not only applies for 'allocate' on the 'target'
construct but it can be also be used on any other directive
inside the target construct, i.e.:
#pragma omp target uses_allocators(omp_cgroup_mem_alloc)
#pragma omp teams reduction(+:xbuf) thread_limit(N) \
allocate(omp_cgroup_mem_alloc:xbuf)
(from the example issue, linked above).
With uses_allocators in particular, it is unclear if
uses_allocators(omp_null_allocator) is allowed or not, IMHO it shouldn't,
but I really don't see a restriction disallowing it.
I think it does not count as predefined allocator and the (new,
post-5.2) wording makes clear that the default allocator (which is
associated with the omp_null_allocator per wording in, e.g., omp_alloc)
is only valid with 'dynamic_allocators'.
Then there is the issue that 5.0/5.1 said for C/C++ that traits-array
should be
"an identifier of const omp_alloctrait_t * type."
which is wrong for multiple reasons, because identifiers don't have type,
expressions or variables do, but more importantly because from the pointer
to const omp_alloctrait_t it is impossible to find out how many elements
the traits have. 5.2 fixed that to say that it must be an array
(so we thankfully know the size), so we certainly should consider that
change like a defect report against 5.0/5.1 too and require even in the
old syntax an array. Note, I'm afraid we need to support even VLAs,
not just constant size arrays.
I concur that 5.2 should be regarded both as fix of old bugs
and syntax extension. (Additionally, as we already support the
5.2 syntax.)
There were some improvements discussed/proposed in
https://github.com/OpenMP/spec/issues/3285
It is a bit difficult to read as I confused two things at the beginning
(mixing allocator expression / traits argument when reading the bullet
points). – But it relates to some of the items you raised here.
There is also in the spec that when allocator in uses_allocators is
a variable, it is treated as a private variable that can't be explicitly
privatized, but nothing said about the traits array, so is say:
void foo () {
omp_allocator_handle_t h;
omp_alloctrait_t t[3] = { ... };
#pragma omp target uses_allocators(h(t)) firstprivate(t)
{
}
ok or not?
(try in addition 'allocate(h : t)' )
We need to firstprivatize t so that we can call
h = omp_init_allocator (omp_default_mem_space, 3, t); in the target region
and it is kind of difficult to privatize the same var multiple times.
I think this relates to the generic question related to
mapping/firstprivatizing const/constexpr/PARAMETER etc.
https://github.com/OpenMP/spec/issues/2158 which really should be fixed.
And yet another issue, in omp_alloctrait_t one can point to other allocators
(with { omp_atk_fallback, some_alloc }). If some_alloc is a predefined
allocator, fine, I don't see big deal with that, especially if that
predefined allocator is
Re: [PATCH, OpenMP, v2] Implement uses_allocators clause for target regions
On Thu, May 19, 2022 at 06:02:43PM +0100, Andrew Stubbs wrote: > On 19/05/2022 17:00, Jakub Jelinek wrote: > > Without requires dynamic_allocators, there are various extra restrictions > > imposed: > > 1) omp_init_allocator/omp_destroy_allocator may not be called (except for > > implicit calls to it from uses_allocators) in a target region > > I interpreted that more like "omp_init_allocator/... is not required to > work", as in the set-up steps provided by dynamic_allocators/uses_allocators > won't be available. Since we don't have any such on/off mode I don't believe > we need to worry about this (and adding extra logic for this is make-work > which will not improve the user-experience). Unfortunately OpenMP as the standard doesn't bother too much with distinctions that e.g. the C/C++ standards make, whether something makes the TU invalid or whether something is only invalid at runtime when reaching it. In any case, I think it would be nice if we diagnosed such uses, doesn't need to be an error, warning would be fine, but help users write portable code, either that they requires dynamic_allocators, or don't and limit themselves to what the standard says should be used in that case. I guess a warning might be better, because we really don't know if it will be actually called at runtime or not from the target region. > > 2) omp_alloc etc. can't be called with omp_null_allocator and the argument > > has to be a constant expression for a predefined memory allocator > > (that is also mentioned on uses_allocators, though that doesn't have to > > be visible in the source because it could be lexically included in > > a target construct's body) > > Again, does a conforming implementation reject this, or is it merely not > required to accept it? I think it is another fuzzy area, but again it would be nice to at least get warnings. Jakub
Re: [PATCH, OpenMP, v2] Implement uses_allocators clause for target regions
On Tue, May 10, 2022 at 07:29:23PM +0800, Chung-Lin Tang wrote:
> @@ -15624,6 +15626,233 @@ c_parser_omp_clause_allocate (c_parser *parser,
> tree list)
>return nl;
> }
>
> +/* OpenMP 5.2:
> + uses_allocators ( allocator-list )
As uses_allocators is a 5.0 feature already, the above should say
/* OpenMP 5.0:
> +
> + allocator-list:
> + allocator
> + allocator , allocator-list
> + allocator ( traits-array )
> + allocator ( traits-array ) , allocator-list
> +
And here it should add
OpenMP 5.2:
> + uses_allocators ( modifier : allocator )
> + uses_allocators ( modifier , modifier : allocator )
> +
> + modifier:
> + traits ( traits-array )
> + memspace ( mem-space-handle ) */
> +
> +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;
> +
> + bool has_modifiers = false;
> + tree memspace_expr = NULL_TREE;
> + tree traits_var = NULL_TREE;
> +
> + if (c_parser_next_token_is (parser, CPP_NAME))
> +{
> + c_token *tok = c_parser_peek_token (parser);
> + const char *p = IDENTIFIER_POINTER (tok->value);
> +
> + if (strcmp ("traits", p) == 0 || strcmp ("memspace", p) == 0)
> + {
> + has_modifiers = true;
> + c_parser_consume_token (parser);
> + matching_parens parens2;;
Double ;;, should be just ;
But more importantly, it is more complex.
When you see
uses_allocators(traits or
uses_allocators(memspace
it is not given that it has modifiers. While the 5.0/5.1 syntax had
a restriction that when allocator is not a predefined allocator (and
traits or memspace aren't predefined allocators) it must use ()s with
traits, so
uses_allocators(traits)
uses_allocators(memspace)
uses_allocators(traits,memspace)
are all invalid,
omp_allocator_handle_t traits;
uses_allocators(traits(mytraits))
or
omp_allocator_handle_t memspace;
uses_allocators(memspace(mytraits),omp_default_mem_alloc)
are valid in the old syntax.
So, I'm afraid to find out if the traits or memspace identifier
seen after uses_allocator ( are modifiers or not we need to
peek (for C with c_parser_peek_nth_token_raw) through all the
modifiers whether we see a : and only in that case say they
are modifiers rather than the old style syntax.
> + parens2.require_open (parser);
> +
> + if (c_parser_next_token_is (parser, CPP_NAME)
> + && (c_parser_peek_token (parser)->id_kind == C_ID_ID
> + || c_parser_peek_token (parser)->id_kind == C_ID_TYPENAME))
> + {
> + tok = c_parser_peek_token (parser);
> + t = lookup_name (tok->value);
> +
> + if (t == NULL_TREE)
> + {
> + undeclared_variable (tok->location, tok->value);
> + t = error_mark_node;
> + }
> + else
> + {
> + if (strcmp ("memspace", p) == 0)
I think it would be better to have bool variable whether
it was memspace or traits modifier, so strcmp just once
with each string, not multiple times.
In the 5.2 syntax, for memspace it is clear that it has to be
an identifier that is the predefined namespace, but for
traits it just says it is a variable of alloctrait array type,
it is unclear if it must be an identifier or could be say structure
element or array element etc. Guess something to discuss on omp-lang
and for now pretend it must be an identifier.
> + if (c_parser_next_token_is (parser, CPP_COMMA))
> + {
> + c_parser_consume_token (parser);
> + tok = c_parser_peek_token (parser);
> + const char *q = "";
> + if (c_parser_next_token_is (parser, CPP_NAME))
> + q = IDENTIFIER_POINTER (tok->value);
> + if (strcmp (q, "memspace") != 0 && strcmp (q, "traits") != 0)
> + {
> + c_parser_error (parser, "expected % or
> %");
> + parens.skip_until_found_close (parser);
> + return list;
> + }
I don't really like the modifiers handling not done in a loop.
As I said above, there needs to be some check whether there are modifiers or
not, but once we figure out there are modifiers, it should be done in a loop
with say some mask var on which traits have been already handled to diagnose
duplicates, we don't want to do the parsing code twice.
> + if (has_modifiers)
> +{
> + if (!c_parser_require (parser, CPP_COLON, "expected %<:%>"))
> + {
> + parens.skip_until_found_close (parser);
> + return list;
> + }
> +
> + if (c_parser_next_token_is (parser, CPP_NAME)
> + && c_parser_peek_token (parser)->id_kind == C_ID_ID)
> + {
> + tree t = lookup_name (c_parser_peek_token (parser)->value);
> +
> + if (t == NULL_TREE)
> + {
> + undeclared_variable (c_parser_peek_token
Re: [PATCH, OpenMP, v2] Implement uses_allocators clause for target regions
On 19/05/2022 17:00, Jakub Jelinek wrote:
Without requires dynamic_allocators, there are various extra restrictions
imposed:
1) omp_init_allocator/omp_destroy_allocator may not be called (except for
implicit calls to it from uses_allocators) in a target region
I interpreted that more like "omp_init_allocator/... is not required to
work", as in the set-up steps provided by
dynamic_allocators/uses_allocators won't be available. Since we don't
have any such on/off mode I don't believe we need to worry about this
(and adding extra logic for this is make-work which will not improve the
user-experience).
2) omp_alloc etc. can't be called with omp_null_allocator and the argument
has to be a constant expression for a predefined memory allocator
(that is also mentioned on uses_allocators, though that doesn't have to
be visible in the source because it could be lexically included in
a target construct's body)
Again, does a conforming implementation reject this, or is it merely not
required to accept it?
3) for allocate directive on static vars the above applies plus it has
to be mentioned in uses_allocators
4) for allocate clause e.g. when privatizing stuff or allocate directive
for automatic vars no such restrictions exist
Now, that means that e.g. the user provided uses_allocators without
requires dynamic_allocators are only useful for the case 4), it is unclear
if that was really intended.
With uses_allocators in particular, it is unclear if
uses_allocators(omp_null_allocator) is allowed or not, IMHO it shouldn't,
but I really don't see a restriction disallowing it.
Then there is the issue that 5.0/5.1 said for C/C++ that traits-array
should be
"an identifier of const omp_alloctrait_t * type."
which is wrong for multiple reasons, because identifiers don't have type,
expressions or variables do, but more importantly because from the pointer
to const omp_alloctrait_t it is impossible to find out how many elements
the traits have. 5.2 fixed that to say that it must be an array
(so we thankfully know the size), so we certainly should consider that
change like a defect report against 5.0/5.1 too and require even in the
old syntax an array. Note, I'm afraid we need to support even VLAs,
not just constant size arrays.
We are only implementing 5.0 at this time. If 5.2 is less work and the
only way to achieve correctness then maybe that's the way to go, but in
general, one step at a time, please.
There is also in the spec that when allocator in uses_allocators is
a variable, it is treated as a private variable that can't be explicitly
privatized, but nothing said about the traits array, so is say:
void foo () {
omp_allocator_handle_t h;
omp_alloctrait_t t[3] = { ... };
#pragma omp target uses_allocators(h(t)) firstprivate(t)
{
}
ok or not? We need to firstprivatize t so that we can call
h = omp_init_allocator (omp_default_mem_space, 3, t); in the target region
and it is kind of difficult to privatize the same var multiple times.
And yet another issue, in omp_alloctrait_t one can point to other allocators
(with { omp_atk_fallback, some_alloc }). If some_alloc is a predefined
allocator, fine, I don't see big deal with that, especially if that
predefined allocator is also mentioned in uses_allocators clause (before or
after). But if it is a user allocator, there is no restriction on that, and
no way how to map that, say that there should be some specific ordering
of uses_allocators induced omp_init_allocator calls and that we should
somehow replace the host value with privatized target replacement.
More on the actual patch later.
Thank you.
Re: [PATCH, OpenMP, v2] Implement uses_allocators clause for target regions
On Tue, May 10, 2022 at 07:29:23PM +0800, Chung-Lin Tang wrote:
> I've attached v2 of the patch. Currently in testing.
Just a general rant, the non-requires dynamic_allocators support
seems to be a total mess in the standard. Probably something
that should be discussed on omp-lang.
Allocators can appear in various places, with requires dynamic_allocators
it is all clear, the only allocators required to be constant expressions
(aka predefined allocators) are on allocate for non-automatic variables
(my understanding is that omp_null_allocator isn't valid for those but I
could be wrong), but there are no other requirements imposed (except of
course referencing a destroyed or not yet initialized allocator is UB),
in particular omp_alloc etc. can be passed omp_null_allocator, or a
variable, and similarly for allocate clause etc.
Without requires dynamic_allocators, there are various extra restrictions
imposed:
1) omp_init_allocator/omp_destroy_allocator may not be called (except for
implicit calls to it from uses_allocators) in a target region
2) omp_alloc etc. can't be called with omp_null_allocator and the argument
has to be a constant expression for a predefined memory allocator
(that is also mentioned on uses_allocators, though that doesn't have to
be visible in the source because it could be lexically included in
a target construct's body)
3) for allocate directive on static vars the above applies plus it has
to be mentioned in uses_allocators
4) for allocate clause e.g. when privatizing stuff or allocate directive
for automatic vars no such restrictions exist
Now, that means that e.g. the user provided uses_allocators without
requires dynamic_allocators are only useful for the case 4), it is unclear
if that was really intended.
With uses_allocators in particular, it is unclear if
uses_allocators(omp_null_allocator) is allowed or not, IMHO it shouldn't,
but I really don't see a restriction disallowing it.
Then there is the issue that 5.0/5.1 said for C/C++ that traits-array
should be
"an identifier of const omp_alloctrait_t * type."
which is wrong for multiple reasons, because identifiers don't have type,
expressions or variables do, but more importantly because from the pointer
to const omp_alloctrait_t it is impossible to find out how many elements
the traits have. 5.2 fixed that to say that it must be an array
(so we thankfully know the size), so we certainly should consider that
change like a defect report against 5.0/5.1 too and require even in the
old syntax an array. Note, I'm afraid we need to support even VLAs,
not just constant size arrays.
There is also in the spec that when allocator in uses_allocators is
a variable, it is treated as a private variable that can't be explicitly
privatized, but nothing said about the traits array, so is say:
void foo () {
omp_allocator_handle_t h;
omp_alloctrait_t t[3] = { ... };
#pragma omp target uses_allocators(h(t)) firstprivate(t)
{
}
ok or not? We need to firstprivatize t so that we can call
h = omp_init_allocator (omp_default_mem_space, 3, t); in the target region
and it is kind of difficult to privatize the same var multiple times.
And yet another issue, in omp_alloctrait_t one can point to other allocators
(with { omp_atk_fallback, some_alloc }). If some_alloc is a predefined
allocator, fine, I don't see big deal with that, especially if that
predefined allocator is also mentioned in uses_allocators clause (before or
after). But if it is a user allocator, there is no restriction on that, and
no way how to map that, say that there should be some specific ordering
of uses_allocators induced omp_init_allocator calls and that we should
somehow replace the host value with privatized target replacement.
More on the actual patch later.
Jakub
