On Wed, Dec 09, 2020 at 05:37:24PM +0000, Kwok Cheung Yeung wrote: > --- a/gcc/c/c-typeck.c > +++ b/gcc/c/c-typeck.c > @@ -14942,6 +14942,11 @@ c_finish_omp_clauses (tree clauses, enum > c_omp_region_type ort) > pc = &OMP_CLAUSE_CHAIN (c); > continue; > > + case OMP_CLAUSE_DETACH: > + t = OMP_CLAUSE_DECL (c); > + pc = &OMP_CLAUSE_CHAIN (c); > + continue; > +
If you wouldn't need to do anything for C for the detach clause, just would just add: case OMP_CLAUSE_DETACH: at the end of the case list that starts below: > case OMP_CLAUSE_IF: > case OMP_CLAUSE_NUM_THREADS: > case OMP_CLAUSE_NUM_TEAMS: But you actually do need to do something, even for C. There are two restrictions: - At most one detach clause can appear on the directive. - If a detach clause appears on the directive, then a mergeable clause cannot appear on the same directive. that should be checked and diagnosed. One place to do that would be like usually in all the FEs separately, that would mean adding bool mergeable_seen = false, detach_seen = false; vars and for those clauses setting the *_seen, plus for DETACH already complain if detach_seen is already true and remove the clause. And at the end of the loop if mergeable_seen && detach_seen, diagnose and remove one of them (perhaps better detach clause). There is the optional second loop that can be used for the removal... Testcase coverage should include: #pragma omp task detach (x) detach (y) as well as #pragma omp task mergeable detach (x) and #pragma omp task detach (x) mergeable (and likewise for Fortran). > + if (cp_lexer_next_token_is_not (parser->lexer, CPP_NAME)) > + { > + cp_parser_error (parser, "expected identifier"); > + return list; > + } > + > + location_t id_loc = cp_lexer_peek_token (parser->lexer)->location; > + tree t, identifier = cp_parser_identifier (parser); > + > + if (identifier == error_mark_node) > + t = error_mark_node; > + else > + { > + t = cp_parser_lookup_name_simple > + (parser, identifier, > + cp_lexer_peek_token (parser->lexer)->location); > + if (t == error_mark_node) > + cp_parser_name_lookup_error (parser, identifier, t, NLE_NULL, > + id_loc); The above doesn't match what cp_parser_omp_var_list_no_open does, in particular it should use cp_parser_id_expression instead of cp_parser_identifier etc. > + else > + { > + tree type = TYPE_MAIN_VARIANT (TREE_TYPE (t)); > + if (!INTEGRAL_TYPE_P (type) > + || TREE_CODE (type) != ENUMERAL_TYPE > + || DECL_NAME (TYPE_NAME (type)) > + != get_identifier ("omp_event_handle_t")) > + { > + error_at (id_loc, "%<detach%> clause event handle " > + "has type %qT rather than " > + "%<omp_event_handle_t%>", > + type); > + return list; You can't do this here for C++, it needs to be done in finish_omp_clauses instead and only be done if the type is not a dependent type. Consider (e.g. should be in testsuite) template <typename T> void foo () { T t; #pragma omp task detach (t) ; } template <typename T> void bar () { T t; #pragma omp task detach (t) ; } void baz () { foo <omp_event_handle_t> (); bar <int> (); // Instantiating this should error } > @@ -7394,6 +7394,9 @@ finish_omp_clauses (tree clauses, enum > c_omp_region_type ort) > } > } > break; > + case OMP_CLAUSE_DETACH: > + t = OMP_CLAUSE_DECL (c); > + break; > Again, restriction checking here, plus check the type if it is non-dependent, otherwise defer that checking for finish_omp_clauses when it will not be dependent anymore. I think you need to handle OMP_CLAUSE_DETACH in cp/pt.c too. > --- a/gcc/gimplify.c > +++ b/gcc/gimplify.c > @@ -9733,6 +9733,19 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq > *pre_p, > } > break; > > + case OMP_CLAUSE_DETACH: > + decl = OMP_CLAUSE_DECL (c); > + if (outer_ctx) > + { > + splay_tree_node on > + = splay_tree_lookup (outer_ctx->variables, > + (splay_tree_key)decl); > + if (on == NULL || (on->value & GOVD_DATA_SHARE_CLASS) == 0) > + omp_firstprivatize_variable (outer_ctx, decl); > + omp_notice_variable (outer_ctx, decl, true); > + } > + break; I don't understand this. My reading of: "The event-handle will be considered as if it was specified on a firstprivate clause. The use of a variable in a detach clause expression of a task construct causes an implicit reference to the variable in all enclosing constructs." is that we should do: case OMP_CLAUSE_DETACH: decl = OMP_CLAUSE_DECL (c); goto do_notice; which does the second sentence, and for the first sentence I believe it talks about the task construct rather than about the outer construct. So (again, something for testsuite): void foo (void) { omp_event_handle_t t; #pragma omp parallel master default (none) /* { dg-error "..." } */ { #pragma omp task detach (t) ; } } The dg-error should be the usual error about t being referenced in the construct but not specified in the data sharing clauses on parallel. And then void bar (void) { omp_event_handle_t t; #pragma omp task detach (t) default (none) omp_fullfill_event (t); // This should be ok, above first sentence says // that it is as if firstprivate (t) } But I think it is actually even stronger than that, #pragma omp task detach (t) firstprivate (t) and #pragma omp task detach (t) shared (t) etc. should be invalid too (at least in pedantic reading). I guess we should ask on omp-lang. If it actually works as firstprivate (t), perhaps we should handle it that way already in the FEs. > --- a/gcc/omp-expand.c > +++ b/gcc/omp-expand.c > @@ -762,6 +762,7 @@ expand_task_call (struct omp_region *region, basic_block > bb, > tree depend = omp_find_clause (clauses, OMP_CLAUSE_DEPEND); > tree finalc = omp_find_clause (clauses, OMP_CLAUSE_FINAL); > tree priority = omp_find_clause (clauses, OMP_CLAUSE_PRIORITY); > + tree detach = omp_find_clause (clauses, OMP_CLAUSE_DETACH); > > unsigned int iflags > = (untied ? GOMP_TASK_FLAG_UNTIED : 0) > @@ -853,6 +854,11 @@ expand_task_call (struct omp_region *region, basic_block > bb, > priority = integer_zero_node; > > gsi = gsi_last_nondebug_bb (bb); > + > + detach = detach > + ? fold_convert (pointer_sized_int_node, OMP_CLAUSE_DETACH_EXPR > (detach)) > + : null_pointer_node; Formatting is wrong, would need to be detach = (detach ? fold_convert (pointer_sized_int_node, OMP_CLAUSE_DETACH_EXPR (detach)) : ...); Now, null_pointer_node doesn't have pointer_sized_int type, so there would be type mismatch. But additionally, let's talk about how it should be implemented. I'd say best would be if: #pragma omp task detach (x) ; expands as GOMP_task (..., ..., ..., ..., ..., ..., other_flags | GOMP_TASK_FLAG_DETACH, ..., ..., &x); where GOMP_task allocates the structure for the struct gomp_task the way it usually does, will have something in that structure include a gomp_sem_t and and a flag that detach is needed (and if detach is needed gomp_sem_init that semaphore) and stores to *detach the address of that semaphore or so. Then you don't need a separate call and allocate something separately. The GOMP_TASK_FLAG_DETACH is needed, because we keep extending GOMP_task rather than creating new versions of that, and so to stay (at least on ABIs I care about) ABI compatible we must not assume detach will have meaningful value if GOMP_TASK_FLAG_DETACH bit is not set in flags. I think we can make the 10th argument to BUILT_IN_GOMP_TASK just void * e.g. to simplify omp-builtins.def and builtin-types.def etc., and just cast it to omp_eventhandler_t * inside of GOMP_task. And, when we pass &x to GOMP_task, we need the detach decl to be addressable, so best in the FEs ensure it is addressable by calling *mark_addressable there. > --- a/gcc/tree.c > +++ b/gcc/tree.c > @@ -299,6 +299,7 @@ unsigned const char omp_clause_num_ops[] = > 1, /* OMP_CLAUSE_LINK */ > 2, /* OMP_CLAUSE_FROM */ > 2, /* OMP_CLAUSE_TO */ > + 1, /* OMP_CLAUSE_DETACH */ > 2, /* OMP_CLAUSE_MAP */ > 1, /* OMP_CLAUSE_USE_DEVICE_PTR */ > 1, /* OMP_CLAUSE_USE_DEVICE_ADDR */ This position for OMP_CLAUSE_DETACH is incorrect (but so is USE_DEVICE_*/IS_DEVICE*/INCLUSIVE/EXCLUSIVE it seems), given: #define OMP_CLAUSE_SIZE(NODE) \ OMP_CLAUSE_OPERAND (OMP_CLAUSE_RANGE_CHECK (OMP_CLAUSE_CHECK (NODE), \ OMP_CLAUSE_FROM, \ OMP_CLAUSE__CACHE_), 1) I think for OMP_CLAUSE_SIZE we just want from/to/map/_cache_ and nothing else, so I'd move detach/use*/is*/*clusive right before from clause. Here 3 times and in tree-core.h to match. We want OMP_CLAUSE_DECL to work for them, which is OMP_CLAUSE_PRIVATE .. OMP_CLAUSE__SCANTEMP_. > +#define OMP_CLAUSE_DETACH_EXPR(NODE) \ > + OMP_CLAUSE_OPERAND (OMP_CLAUSE_SUBCODE_CHECK (NODE, OMP_CLAUSE_DETACH),0) No need for that, OMP_CLAUSE_DECL can be used for that. It is not an expression, but decl anyway. I don't see tree-nested.c changes, that is needed too. > @@ -556,6 +557,13 @@ > end interface > > interface > + subroutine omp_fulfill_event (event) > + use omp_lib_kinds > + integer (kind=omp_event_handle_kind), value, intent(in) :: event Too long line? > diff --git a/libgomp/priority_queue.h b/libgomp/priority_queue.h > index 0ad78f5..c6fd80d 100644 > --- a/libgomp/priority_queue.h > +++ b/libgomp/priority_queue.h > @@ -113,6 +113,8 @@ enum priority_queue_type > PQ_IGNORED = 999 > }; > > +typedef bool (*priority_queue_predicate)(struct gomp_task *); Formatting, space before (. > + > /* Priority queue implementation prototypes. */ > > extern bool priority_queue_task_in_queue_p (enum priority_queue_type, > @@ -122,6 +124,9 @@ extern void priority_queue_dump (enum priority_queue_type, > struct priority_queue *); > extern void priority_queue_verify (enum priority_queue_type, > struct priority_queue *, bool); > +extern struct gomp_task *priority_queue_find (enum priority_queue_type, > + struct priority_queue *, > + priority_queue_predicate); > extern void priority_tree_remove (enum priority_queue_type, > struct priority_queue *, > struct priority_node *); > diff --git a/libgomp/task.c b/libgomp/task.c > index a95067c..ae1fcf7 100644 > --- a/libgomp/task.c > +++ b/libgomp/task.c > @@ -29,6 +29,7 @@ > #include "libgomp.h" > #include <stdlib.h> > #include <string.h> > +#include <stdio.h> > #include "gomp-constants.h" > > typedef struct gomp_task_depend_entry *hash_entry_type; > @@ -86,6 +87,7 @@ gomp_init_task (struct gomp_task *task, struct gomp_task > *parent_task, > task->dependers = NULL; > task->depend_hash = NULL; > task->depend_count = 0; > + task->detach_event = NULL; > } > > /* Clean up a task, after completing it. */ > @@ -326,6 +328,28 @@ gomp_task_handle_depend (struct gomp_task *task, struct > gomp_task *parent, > } > } > > +uintptr_t > +GOMP_new_event () > +{ > + struct gomp_allow_completion_event *event; > + > + event = (struct gomp_allow_completion_event *) > + gomp_malloc (sizeof (struct gomp_allow_completion_event)); > + event->fulfilled = false; > + gomp_sem_init (&event->completion_sem, 0); > + > + gomp_debug (0, "GOMP_new_event: %p\n", event); > + > + return (uintptr_t) event; > +} > + > +static bool > +task_fulfilled_p (struct gomp_task *task) > +{ > + return __atomic_load_n (&task->detach_event->fulfilled, > + __ATOMIC_RELAXED); > +} I don't understand the fulfilled and sem duality, I think there should be a flag that the flag has detach at all, and then just a semaphore, the semaphore state itself would indicate if it is fulfilled or not. > @@ -347,11 +371,14 @@ gomp_task_handle_depend (struct gomp_task *task, struct > gomp_task *parent, > void > GOMP_task (void (*fn) (void *), void *data, void (*cpyfn) (void *, void *), > long arg_size, long arg_align, bool if_clause, unsigned flags, > - void **depend, int priority) > + void **depend, int priority, uintptr_t detach) > { > struct gomp_thread *thr = gomp_thread (); > struct gomp_team *team = thr->ts.team; > > + struct gomp_allow_completion_event *detach_event = > + detach ? (struct gomp_allow_completion_event *) detach : NULL; Formatting (no = at the end of line), but as I said above, I'd prefer to do it differently. > +void omp_fulfill_event(omp_event_handle_t event) Formatting. omp_fulfill_event on separate line from void, and space before (. > +{ > + struct gomp_allow_completion_event *ev = > + (struct gomp_allow_completion_event *) event; Formatting, no = at the end of line and the (struct ... is indented wierdly. Jakub