On Fri, Jan 15, 2021 at 03:07:56PM +0000, Kwok Cheung Yeung wrote:
> + {
> + tree detach_decl = OMP_CLAUSE_DECL (*detach_seen);
> +
> + for (pc = &clauses, c = clauses; c ; c = *pc)
> + {
> + bool remove = false;
> + if ((OMP_CLAUSE_CODE (c) == OMP_CLAUSE_SHARED
> + || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_PRIVATE
> + || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_FIRSTPRIVATE
> + || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_LASTPRIVATE)
> + && OMP_CLAUSE_DECL (c) == detach_decl)
> + {
> + error_at (OMP_CLAUSE_LOCATION (c),
> + "the event handle of a %<detach%> clause "
> + "should not be in a data-sharing clause");
> + remove = true;
> + }
I think you don't need this loop, instead you could just check
if (bitmap_bit_p (&generic_head, DECL_UID (detach_decl))
|| bitmap_bit_p (&firstprivate_head, DECL_UID (detach_decl))
|| bitmap_bit_p (&lastprivate_head, DECL_UID (detach_decl)))
> + || TREE_CODE (type) != ENUMERAL_TYPE
|| DECL_NAME (TYPE_NAME (type))
!= get_identifier ("omp_event_handle_t")))
The formatting is off, and I think as a service to Emacs users we are
usually formatting it as:
|| (DECL_NAME (TYPE_NAME (type))
!= get_identifier ("omp_event_handle_t"))))
> +
> + detach = detach
> + ? build_fold_addr_expr (OMP_CLAUSE_DECL (detach))
> + : null_pointer_node;
Again formatting nit, please write:
detach = (detach
? build_fold_addr_expr (OMP_CLAUSE_DECL (detach))
: null_pointer_node);
> @@ -2416,6 +2421,64 @@ finish_taskreg_scan (omp_context *ctx)
> TYPE_FIELDS (ctx->srecord_type) = f1;
> }
> }
> + if (detach_clause)
> + {
> + tree c, field;
> +
> + /* Look for a firstprivate clause with the detach event handle. */
> + for (c = gimple_omp_taskreg_clauses (ctx->stmt);
> + c; c = OMP_CLAUSE_CHAIN (c))
> + {
> + if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_FIRSTPRIVATE)
> + continue;
> + if (maybe_lookup_decl_in_outer_ctx (OMP_CLAUSE_DECL (c), ctx)
> + == OMP_CLAUSE_DECL (detach_clause))
> + break;
> + }
> +
> + if (c)
> + field = lookup_field (OMP_CLAUSE_DECL (c), ctx);
> + else
> + {
> + /* The detach event handle is not referenced within the
> + task context, so add a temporary field for it here. */
> + field = build_decl (OMP_CLAUSE_LOCATION (detach_clause),
> + FIELD_DECL, NULL_TREE, ptr_type_node);
> + insert_field_into_struct (ctx->record_type, field);
Can't you just force the firstprivate clause during gimplification, so that
it doesn't go away even if not referenced?
That would mean just forcing in | GOVD_SEEN when it is added.
If not, not a big deal, just thought it could be easier.
> +
> + if (ctx->srecord_type)
> + {
> + tree sfield
> + = build_decl (OMP_CLAUSE_LOCATION (detach_clause),
> + FIELD_DECL, NULL_TREE, ptr_type_node);
> + insert_field_into_struct (ctx->srecord_type, sfield);
> + }
> + }
> +
> + /* Move field corresponding to the detach clause first.
> + This is filled by GOMP_task and needs to be in a
> + specific position. */
> + p = &TYPE_FIELDS (ctx->record_type);
> + while (*p)
> + if (*p == field)
> + *p = DECL_CHAIN (*p);
> + else
> + p = &DECL_CHAIN (*p);
> + DECL_CHAIN (field) = TYPE_FIELDS (ctx->record_type);
> + TYPE_FIELDS (ctx->record_type) = field;
> + if (ctx->srecord_type)
> + {
> + field = lookup_sfield (OMP_CLAUSE_DECL (detach_clause), ctx);
> + p = &TYPE_FIELDS (ctx->srecord_type);
> + while (*p)
> + if (*p == field)
> + *p = DECL_CHAIN (*p);
> + else
> + p = &DECL_CHAIN (*p);
> + DECL_CHAIN (field) = TYPE_FIELDS (ctx->srecord_type);
> + TYPE_FIELDS (ctx->srecord_type) = field;
> + }
> + }
> layout_type (ctx->record_type);
> fixup_child_record_type (ctx);
> if (ctx->srecord_type)
> diff --git a/gcc/testsuite/c-c++-common/gomp/task-detach-1.c
> b/gcc/testsuite/c-c++-common/gomp/task-detach-1.c
> new file mode 100644
> index 0000000..c7dda82
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/gomp/task-detach-1.c
> @@ -0,0 +1,32 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fopenmp" } */
> +
> +#include <omp.h>
> +
> +void f (omp_event_handle_t x, omp_event_handle_t y, int z)
> +{
> + #pragma omp task detach (x) detach (y) /* { dg-error "there can be at most
> one 'detach' clause in a task construct" } */
It would be on a task construct rather than in a task construct, but the
common wording for this diagnostics is
"too many %qs clauses", "detach"
Please use that wording.
> +
> +template <typename T>
> +void func ()
> +{
> + T t;
> + #pragma omp task detach (t) // { dg-error "'detach' clause event handle
> has type 'int' rather than 'omp_event_handle_t'" }
> + ;
> +}
> +
> +void f()
> +{
> + func <omp_event_handle_t> ();
> + func <int> (); // { dg-message "required from here" }
> +}
I'd suggest two different templates with the same content, because the
dg-error is in the template and splitting them might make it clearer in
which one it is from. The required from here message is I think ignored by
default, so if it incorrectly diagnosed both spots, one wouldn't notice.
> +
> + !$omp task detach (x) firstprivate (x) ! { dg-error "DETACH event handle
> 'x' in FIRSTPRIVATE clause at \\\(1\\\)" }
> + !$omp end task
> +
> + !$omp task detach (x) shared (x) ! { dg-error "DETACH event handle 'x' in
> SHARED clause at \\\(1\\\)" }
> + !$omp end task
> +end program
> \ No newline at end of file
Please add newline.
> +
> + gomp_debug(0, "omp_fulfill_event: %p\n", sem);
Space before (.
Otherwise LGTM, thanks.
Jakub
