Thanks for the review.
On 16/01/2021 9:25 am, Jakub Jelinek wrote:
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)))
I think the main problem with this is that you cannot then point to the location
of the offending data-sharing clause. Given a task construct with 'detach(x)
shared(x)', I would tend to think of the 'shared(x)' as being the incorrect part
here, and so would want the error to point to it. Unless you have any
objections, I am inclined to keep this as it is?
@@ -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.
I've tried this diff:
case OMP_CLAUSE_DETACH:
- decl = OMP_CLAUSE_DECL (c);
- goto do_notice;
+ flags = GOVD_FIRSTPRIVATE | GOVD_SEEN;
+ goto do_add;
and just asserted that a suitable firstprivate clause is found in
finish_taskreg_scan, and it seems to work fine :-).
+ #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.
Done, though I don't see the point of using a %qs format code with a constant
string here...
I have applied your other suggestions, and have retested the gomp.exp and
libgomp tests. The full testrun started yesterday showed no regressions. If you
have no further issues then I will commit this later tonight ahead of stage4.
Thanks
Kwok
commit 68f17e5d3f28b4150fc0fa9112671403c4519c05
Author: Kwok Cheung Yeung <k...@codesourcery.com>
Date: Sat Jan 16 09:27:28 2021 -0800
More task detach fixes.
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 4e9b21b..b938e6a 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -14942,8 +14942,7 @@ c_finish_omp_clauses (tree clauses, enum
c_omp_region_type ort)
if (detach_seen)
{
error_at (OMP_CLAUSE_LOCATION (c),
- "there can be at most one %<detach%> clause in a "
- "task construct");
+ "too many %<detach%> clauses on a task construct");
remove = true;
break;
}
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 9dfaea2..c28cde0 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -7425,8 +7425,7 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type
ort)
if (detach_seen)
{
error_at (OMP_CLAUSE_LOCATION (c),
- "there can be at most one %<detach%> clause in a "
- "task construct");
+ "too many %<detach%> clauses on a task construct");
remove = true;
break;
}
@@ -7436,8 +7435,8 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type
ort)
if (!type_dependent_expression_p (t)
&& (!INTEGRAL_TYPE_P (type)
|| TREE_CODE (type) != ENUMERAL_TYPE
- || DECL_NAME (TYPE_NAME (type))
- != get_identifier ("omp_event_handle_t")))
+ || (DECL_NAME (TYPE_NAME (type))
+ != get_identifier ("omp_event_handle_t"))))
{
error_at (OMP_CLAUSE_LOCATION (c),
"%<detach%> clause event handle "
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 5fbe2fc..d2ac5f9 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -9764,8 +9764,8 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq
*pre_p,
break;
case OMP_CLAUSE_DETACH:
- decl = OMP_CLAUSE_DECL (c);
- goto do_notice;
+ flags = GOVD_FIRSTPRIVATE | GOVD_SEEN;
+ goto do_add;
case OMP_CLAUSE_IF:
if (OMP_CLAUSE_IF_MODIFIER (c) != ERROR_MARK
diff --git a/gcc/omp-expand.c b/gcc/omp-expand.c
index ea0f058..7559ec8 100644
--- a/gcc/omp-expand.c
+++ b/gcc/omp-expand.c
@@ -860,9 +860,9 @@ expand_task_call (struct omp_region *region, basic_block bb,
gsi = gsi_last_nondebug_bb (bb);
- detach = detach
- ? build_fold_addr_expr (OMP_CLAUSE_DECL (detach))
- : null_pointer_node;
+ detach = (detach
+ ? build_fold_addr_expr (OMP_CLAUSE_DECL (detach))
+ : null_pointer_node);
tree t = gimple_omp_task_data_arg (entry_stmt);
if (t == NULL)
diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index 60a651f..c1267dc 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -2436,24 +2436,8 @@ finish_taskreg_scan (omp_context *ctx)
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);
-
- 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);
- }
- }
+ gcc_assert (c);
+ field = lookup_field (OMP_CLAUSE_DECL (c), ctx);
/* Move field corresponding to the detach clause first.
This is filled by GOMP_task and needs to be in a
diff --git a/gcc/testsuite/c-c++-common/gomp/task-detach-1.c
b/gcc/testsuite/c-c++-common/gomp/task-detach-1.c
index f50f748..4558bc1 100644
--- a/gcc/testsuite/c-c++-common/gomp/task-detach-1.c
+++ b/gcc/testsuite/c-c++-common/gomp/task-detach-1.c
@@ -10,7 +10,7 @@ extern void omp_fulfill_event (omp_event_handle_t);
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" } */
+ #pragma omp task detach (x) detach (y) /* { dg-error "too many 'detach'
clauses on a task construct" } */
;
#pragma omp task mergeable detach (x) /* { dg-error "'detach' clause must
not be used together with 'mergeable' clause" } */
diff --git a/gcc/testsuite/g++.dg/gomp/task-detach-1.C
b/gcc/testsuite/g++.dg/gomp/task-detach-1.C
index 2f0c650..6028cb4 100644
--- a/gcc/testsuite/g++.dg/gomp/task-detach-1.C
+++ b/gcc/testsuite/g++.dg/gomp/task-detach-1.C
@@ -7,7 +7,15 @@ typedef enum omp_event_handle_t
} omp_event_handle_t;
template <typename T>
-void func ()
+void foo ()
+{
+ T t;
+ #pragma omp task detach (t)
+ ;
+}
+
+template <typename T>
+void bar ()
{
T t;
#pragma omp task detach (t) // { dg-error "'detach' clause event handle has
type 'int' rather than 'omp_event_handle_t'" }
@@ -16,6 +24,6 @@ void func ()
void f()
{
- func <omp_event_handle_t> ();
- func <int> (); // { dg-message "required from here" }
+ foo <omp_event_handle_t> ();
+ bar <int> (); // { dg-message "required from here" }
}
diff --git a/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90
b/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90
index 114068e..4763f13 100644
--- a/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90
+++ b/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90
@@ -26,4 +26,4 @@ program task_detach_1
!$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
+end program
diff --git a/libgomp/task.c b/libgomp/task.c
index f02c1ea..5ece878 100644
--- a/libgomp/task.c
+++ b/libgomp/task.c
@@ -2409,7 +2409,7 @@ omp_fulfill_event (omp_event_handle_t event)
if (__atomic_load_n (sem, __ATOMIC_RELAXED))
gomp_fatal ("omp_fulfill_event: %p event already fulfilled!\n", sem);
- gomp_debug(0, "omp_fulfill_event: %p\n", sem);
+ gomp_debug (0, "omp_fulfill_event: %p\n", sem);
gomp_sem_post (sem);
if (team)
gomp_team_barrier_wake (&team->barrier, 1);
