Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
* Jakub Jelinek: > Not all targets support GNU style function versioning where one can have > multiple symbol versions for the same symbol name, and I wanted to > avoid GOMP_task2, GOMP_task3 etc. Why? If it is about aesthetics, wouldn't compliance with language standards and ABI requirements be more important? > In retrospect, it would have been probably better to make the function > void (*fn) (void *), /* ... */, unsigned flags, ...) > but it is too late to change that now. Right, on some targets, the caller would have to allocate a parameter save area, resulting in stack corruption if it is missing. > On the other side, I really don't see a reason why gcc would try to spill > something that has never been changed. If the argument slot is dead, GCC might reuse it for something else (if the ABI says that the callee owns those slots). Thanks, Florian -- Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On Thu, Jan 21, 2021 at 08:32:52AM +0100, Thomas Schwinge wrote: > > and which of those depend, priority and detach argument is present depends > > on the bits in flags. > > I'm afraid the compiler just decided to spill the detach = NULL store in > > if ((flags & GOMP_TASK_FLAG_DETACH) == 0) > > detach = NULL; > > on s390x into the argument stack slot. Not a problem if the caller passes > > all those 10 arguments, but if not, can clobber random stack location. > > > > This hack should fix it up. Priority doesn't need changing, but I've > > changed it anyway just to be safe. With the patch none of the 3 arguments > > are ever modified, so I'd hope gcc doesn't decide to spill something > > unrelated there. > > That still seems fragile; is "hope gcc doesn't decide to spill" really > sufficient? > > Cannot we (easily) use symbol versioning to introduce new entry point > variants (which then internally all route to the same function)? Not all targets support GNU style function versioning where one can have multiple symbol versions for the same symbol name, and I wanted to avoid GOMP_task2, GOMP_task3 etc. In retrospect, it would have been probably better to make the function void (*fn) (void *), /* ... */, unsigned flags, ...) but it is too late to change that now. And we can't really change at least the GOMP_task with just flags, with also depend and with also priority, so we have that potential problem anyway. On the other side, I really don't see a reason why gcc would try to spill something that has never been changed. Jakub
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
Hi!
On 2021-01-20T17:40:58+0100, Jakub Jelinek via Gcc-patches
wrote:
> On Wed, Jan 20, 2021 at 05:04:39PM +0100, Florian Weimer wrote:
>> Sorry, this appears to cause OpenMP task state corruption in RPM. We
>> have only seen this on s390x.
>
> Haven't actually verified it, but my suspection is that this is a caller
> stack corruption.
>
> We play with fire with the GOMP_task API/ABI extensions, the GOMP_task
> function used to be:
> void
> GOMP_task (void (*fn) (void *), void *data, void (*cpyfn) (void *, void *),
>long arg_size, long arg_align, bool if_clause, unsigned flags);
> and later:
> 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);
> and later:
> 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);
> and now:
> 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 *detach)
Yeah, I'd wondered about that, too.
> and which of those depend, priority and detach argument is present depends
> on the bits in flags.
> I'm afraid the compiler just decided to spill the detach = NULL store in
> if ((flags & GOMP_TASK_FLAG_DETACH) == 0)
> detach = NULL;
> on s390x into the argument stack slot. Not a problem if the caller passes
> all those 10 arguments, but if not, can clobber random stack location.
>
> This hack should fix it up. Priority doesn't need changing, but I've
> changed it anyway just to be safe. With the patch none of the 3 arguments
> are ever modified, so I'd hope gcc doesn't decide to spill something
> unrelated there.
That still seems fragile; is "hope gcc doesn't decide to spill" really
sufficient?
Cannot we (easily) use symbol versioning to introduce new entry point
variants (which then internally all route to the same function)?
Grüße
Thomas
> 2021-01-20 Jakub Jelinek
>
> * task.c (GOMP_task): Rename priority argument to priority_arg,
> add priority automatic variable and modify that variable. Instead of
> clearing detach argument when GOMP_TASK_FLAG_DETACH bit is not set,
> check flags for that bit.
>
> --- libgomp/task.c.jj 2021-01-18 07:18:42.362339622 +0100
> +++ libgomp/task.c2021-01-20 17:23:36.973758174 +0100
> @@ -354,10 +354,11 @@ task_fulfilled_p (struct gomp_task *task
> 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 *detach)
> +void **depend, int priority_arg, void *detach)
> {
>struct gomp_thread *thr = gomp_thread ();
>struct gomp_team *team = thr->ts.team;
> + int priority = 0;
>
> #ifdef HAVE_BROKEN_POSIX_SEMAPHORES
>/* If pthread_mutex_* is used for omp_*lock*, then each task must be
> @@ -385,13 +386,12 @@ GOMP_task (void (*fn) (void *), void *da
> }
> }
>
> - if ((flags & GOMP_TASK_FLAG_PRIORITY) == 0)
> -priority = 0;
> - else if (priority > gomp_max_task_priority_var)
> -priority = gomp_max_task_priority_var;
> -
> - if ((flags & GOMP_TASK_FLAG_DETACH) == 0)
> -detach = NULL;
> + if (__builtin_expect ((flags & GOMP_TASK_FLAG_PRIORITY) != 0, 0))
> +{
> + priority = priority_arg;
> + if (priority > gomp_max_task_priority_var)
> + priority = gomp_max_task_priority_var;
> +}
>
>if (!if_clause || team == NULL
>|| (thr->task && thr->task->final_task)
> @@ -415,7 +415,7 @@ GOMP_task (void (*fn) (void *), void *da
> || (flags & GOMP_TASK_FLAG_FINAL);
>task.priority = priority;
>
> - if (detach)
> + if ((flags & GOMP_TASK_FLAG_DETACH) != 0)
> {
> task.detach = true;
> gomp_sem_init (&task.completion_sem, 0);
> @@ -443,7 +443,7 @@ GOMP_task (void (*fn) (void *), void *da
>else
> fn (data);
>
> - if (detach && !task_fulfilled_p (&task))
> + if (task.detach && !task_fulfilled_p (&task))
> gomp_sem_wait (&task.completion_sem);
>
>/* Access to "children" is normally done inside a task_lock
> @@ -484,7 +484,7 @@ GOMP_task (void (*fn) (void *), void *da
>task->kind = GOMP_TASK_UNDEFERRED;
>task->in_tied_task = parent->in_tied_task;
>task->taskgroup = taskgroup;
> - if (detach)
> + if ((flags & GOMP_TASK_FLAG_DETACH) != 0)
> {
> task->detach = true;
> gomp_sem_init (&task->completion_sem, 0);
>
>
> Jakub
-
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander
Walter
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On Wed, Jan 20, 2021 at 05:04:39PM +0100, Florian Weimer wrote:
> Sorry, this appears to cause OpenMP task state corruption in RPM. We
> have only seen this on s390x.
Haven't actually verified it, but my suspection is that this is a caller
stack corruption.
We play with fire with the GOMP_task API/ABI extensions, the GOMP_task
function used to be:
void
GOMP_task (void (*fn) (void *), void *data, void (*cpyfn) (void *, void *),
long arg_size, long arg_align, bool if_clause, unsigned flags);
and later:
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);
and later:
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);
and now:
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 *detach)
and which of those depend, priority and detach argument is present depends
on the bits in flags.
I'm afraid the compiler just decided to spill the detach = NULL store in
if ((flags & GOMP_TASK_FLAG_DETACH) == 0)
detach = NULL;
on s390x into the argument stack slot. Not a problem if the caller passes
all those 10 arguments, but if not, can clobber random stack location.
This hack should fix it up. Priority doesn't need changing, but I've
changed it anyway just to be safe. With the patch none of the 3 arguments
are ever modified, so I'd hope gcc doesn't decide to spill something
unrelated there.
2021-01-20 Jakub Jelinek
* task.c (GOMP_task): Rename priority argument to priority_arg,
add priority automatic variable and modify that variable. Instead of
clearing detach argument when GOMP_TASK_FLAG_DETACH bit is not set,
check flags for that bit.
--- libgomp/task.c.jj 2021-01-18 07:18:42.362339622 +0100
+++ libgomp/task.c 2021-01-20 17:23:36.973758174 +0100
@@ -354,10 +354,11 @@ task_fulfilled_p (struct gomp_task *task
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 *detach)
+ void **depend, int priority_arg, void *detach)
{
struct gomp_thread *thr = gomp_thread ();
struct gomp_team *team = thr->ts.team;
+ int priority = 0;
#ifdef HAVE_BROKEN_POSIX_SEMAPHORES
/* If pthread_mutex_* is used for omp_*lock*, then each task must be
@@ -385,13 +386,12 @@ GOMP_task (void (*fn) (void *), void *da
}
}
- if ((flags & GOMP_TASK_FLAG_PRIORITY) == 0)
-priority = 0;
- else if (priority > gomp_max_task_priority_var)
-priority = gomp_max_task_priority_var;
-
- if ((flags & GOMP_TASK_FLAG_DETACH) == 0)
-detach = NULL;
+ if (__builtin_expect ((flags & GOMP_TASK_FLAG_PRIORITY) != 0, 0))
+{
+ priority = priority_arg;
+ if (priority > gomp_max_task_priority_var)
+ priority = gomp_max_task_priority_var;
+}
if (!if_clause || team == NULL
|| (thr->task && thr->task->final_task)
@@ -415,7 +415,7 @@ GOMP_task (void (*fn) (void *), void *da
|| (flags & GOMP_TASK_FLAG_FINAL);
task.priority = priority;
- if (detach)
+ if ((flags & GOMP_TASK_FLAG_DETACH) != 0)
{
task.detach = true;
gomp_sem_init (&task.completion_sem, 0);
@@ -443,7 +443,7 @@ GOMP_task (void (*fn) (void *), void *da
else
fn (data);
- if (detach && !task_fulfilled_p (&task))
+ if (task.detach && !task_fulfilled_p (&task))
gomp_sem_wait (&task.completion_sem);
/* Access to "children" is normally done inside a task_lock
@@ -484,7 +484,7 @@ GOMP_task (void (*fn) (void *), void *da
task->kind = GOMP_TASK_UNDEFERRED;
task->in_tied_task = parent->in_tied_task;
task->taskgroup = taskgroup;
- if (detach)
+ if ((flags & GOMP_TASK_FLAG_DETACH) != 0)
{
task->detach = true;
gomp_sem_init (&task->completion_sem, 0);
Jakub
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
* Kwok Cheung Yeung:
> From 788687f87ad41e51258738ce068ee38d7b24defc Mon Sep 17 00:00:00 2001
> From: Kwok Cheung Yeung
> Date: Fri, 15 Jan 2021 04:49:36 -0800
> Subject: [PATCH] openmp: Add support for the OpenMP 5.0 task detach clause
Sorry, this appears to cause OpenMP task state corruption in RPM. We
have only seen this on s390x.
The relevant code looks like this:
745 * (largest first) to help achieve an optimal load distribution.
746 */
747 rpmRC packageBinaries(rpmSpec spec, const char *cookie, int cheating)
748 {
749 rpmRC rc = RPMRC_OK;
750 Package pkg;
751 Package *tasks;
752 int npkgs = 0;
753
754 for (pkg = spec->packages; pkg != NULL; pkg = pkg->next)
755 npkgs++;
756 tasks = xcalloc(npkgs, sizeof(Package));
757
758 pkg = spec->packages;
759 for (int i = 0; i < npkgs; i++) {
760 tasks[i] = pkg;
761 pkg = pkg->next;
762 }
763 qsort(tasks, npkgs, sizeof(Package), compareBinaries);
764
765 #pragma omp parallel
766 #pragma omp single
767 for (int i = 0; i < npkgs; i++) {
768 Package pkg = tasks[i];
769 #pragma omp task untied priority(i)
770 {
771 pkg->rc = packageBinary(spec, pkg, cookie, cheating,
&pkg->filename);
772 rpmlog(RPMLOG_DEBUG,
773 _("Finished binary package job, result %d, filename
%s\n"),
774 pkg->rc, pkg->filename);
775 if (pkg->rc) {
776 #pragma omp critical
777 rc = pkg->rc;
778 }
779 } /* omp task */
780 if (rc)
781 break;
782 }
783
784 /* Now check the package set if enabled */
785 if (rc == RPMRC_OK)
786 rc = checkPackageSet(spec->packages);
787
788 free(tasks);
789
790 return rc;
791 }
Backtrace looks like this:
Thread 1 "rpmbuild" received signal SIGSEGV, Segmentation fault.
0x03fffde89e60 in packageBinaries._omp_fn.0 () at pack.c:780
780 if (rc)
(gdb) bt
#0 0x03fffde89e60 in packageBinaries._omp_fn.0 () at pack.c:780
#1 0x03fffca94806 in GOMP_parallel (
fn=0x3fffde89d80 , data=0x3fff478,
num_threads=2, flags=) at ../../../libgomp/parallel.c:178
#2 0x03fffde953fa in packageBinaries (cheating=0, cookie=0x0,
spec=0x2aa00065570) at pack.c:765
#3 buildSpec (ts=, buildArgs=,
spec=0x2aa00065570, what=) at build.c:411
#4 0x03fffde98074 in rpmSpecBuild (ts=,
spec=, buildArgs=) at build.c:452
#5 0x02aa3e74 in buildForTarget (ts=0x2aa00069a80,
arg=, ba=0x2aa7990 ) at rpmbuild.c:500
#6 0x02aa409a in build (ts=0x2aa00069a80,
arg=0x3fffe3a "/builddir/build/SPECS/compsize.spec", rcfile=0x0,
ba=0x2aa7990 ) at rpmbuild.c:552
#7 0x02aa2f84 in main (argc=, argv=)
at rpmbuild.c:690
Debuginfo is a bit wonky, but the task pointer appears to be null.
Disassembly of the function around the crash site:
(gdb) disassemble
Dump of assembler code for function packageBinaries._omp_fn.0:
0x03fffde89d80 <+0>: stmg%r6,%r15,48(%r15)
0x03fffde89d86 <+6>: ear %r1,%a0
0x03fffde89d8a <+10>:lgr %r14,%r15
0x03fffde89d8e <+14>:lay %r15,-280(%r15)
0x03fffde89d94 <+20>:aghi%r14,-24
0x03fffde89d98 <+24>:std %f8,0(%r14)
0x03fffde89d9c <+28>:std %f10,8(%r14)
0x03fffde89da0 <+32>:std %f14,16(%r14)
0x03fffde89da4 <+36>:sllg%r1,%r1,32
0x03fffde89daa <+42>:ear %r1,%a1
0x03fffde89dae <+46>:l %r11,32(%r2)
0x03fffde89db2 <+50>:stg %r1,200(%r15)
0x03fffde89db8 <+56>:lg %r10,16(%r2)
0x03fffde89dbe <+62>:l %r13,24(%r2)
0x03fffde89dc2 <+66>:mvc 248(8,%r15),40(%r1)
0x03fffde89dc8 <+72>:ld %f8,8(%r2)
0x03fffde89dcc <+76>:ld %f10,0(%r2)
0x03fffde89dd0 <+80>:lgr %r8,%r2
0x03fffde89dd4 <+84>:brasl %r14,0x3fffde87530
0x03fffde89dda <+90>:cije%r2,0,0x3fffde89e76
0x03fffde89de0 <+96>:cijnh %r11,0,0x3fffde89e76
0x03fffde89de6 <+102>: stg %r8,192(%r15)
0x03fffde89dec <+108>: la %r7,208(%r15)
0x03fffde89df0 <+112>: la %r1,28(%r8)
0x03fffde89df4 <+116>: lgr %r8,%r7
0x03fffde89df8 <+120>: lgdr%r7,%f8
0x03fffde89dfc <+124>: ldgr%f14,%r1
0x03fffde89e00 <+128>: lhi %r9,0
0x03fffde89e04 <+132>: lg %r1,0(%r10)
0x03fffde89e0a <+138>: std %f10,208(%r15)
0x03fffde89e0e <+142>: std %f14,224(%r15)
0x03fffde89e12 <+146>: stg %r1,232(%r15)
0x03fffde89e18 <+152>: st %r13,240(%r15)
0x03fffde89e1c <+156>: stg %r7,216(%r15)
0x03f
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
Hi Jakub,
> On Sun, Jan 17, 2021 at 04:25:24PM +0100, Andreas Schwab wrote:
>> On Jan 17 2021, Jakub Jelinek via Gcc-patches wrote:
>>
>> > Kwok, I guess you can reproduce it even on Linux with --disable-linux-futex
>>
>> And all targets that are not explicitly configured in
>> libcomp/configure.tgt, where --enable-linux-futex is a no-op.
>
> Completely untested patch (except for the linux futex version; and RTEMS
> stuff is missing; I think it doesn't have a function for it but has a
> counter in the struct, so perhaps fetch it manually from there), it is
> Sunday, don't want to do more tonight:
this worked for me on both i386-pc-solaris2.11 and
sparc-sun-solaris2.11, thanks. However, I had to rerun the builds with
the DWARF-5 patch backed out since that caused so much breakage that the
results were all but useless.
Two comments, though:
> --- libgomp/config/linux/sem.h.jj 2021-01-04 10:25:56.160037625 +0100
> +++ libgomp/config/linux/sem.h2021-01-17 16:49:39.900750416 +0100
> @@ -85,4 +85,13 @@ gomp_sem_post (gomp_sem_t *sem)
>if (__builtin_expect (count & SEM_WAIT, 0))
> gomp_sem_post_slow (sem);
> }
> +
> +static inline int
> +gomp_sem_getcount (gomp_sem_t *sem)
> +{
> + int count = __atomic_load_n (sem, MEMMODEL_RELAXED);
> + if ((count & SEM_WAIT) != 0)
> +return -1;
> + return count / SEM_INC;
> +}
> #endif /* GOMP_SEM_H */
> --- libgomp/config/posix/sem.h.jj 2021-01-04 10:25:56.166037557 +0100
> +++ libgomp/config/posix/sem.h2021-01-17 16:49:53.605593659 +0100
> @@ -64,6 +64,8 @@ extern void gomp_sem_post (gomp_sem_t *s
>
> extern void gomp_sem_destroy (gomp_sem_t *sem);
>
> +extern int gomp_sem_getcount (gomp_sem_t *sem);
> +
> #else /* HAVE_BROKEN_POSIX_SEMAPHORES */
>
> typedef sem_t gomp_sem_t;
> @@ -84,5 +86,13 @@ static inline void gomp_sem_destroy (gom
> {
>sem_destroy (sem);
> }
> +
> +static inline int gomp_sem_getcount (gomp_sem_t *sem)
Shouldn't there be a line break before gomp_semp_getcount (and once
again in posix/sem.c), as done in linux/sem.h above? libgomp seems a
bit inconsistent in that matter, though.
Besides, I've seen regular timeouts on both Solaris and Linux/x86_64 for
one of the new tests:
WARNING: libgomp.fortran/task-detach-6.f90 -O2 execution test program timed
out.
FAIL: libgomp.fortran/task-detach-6.f90 -O2 execution test
It doesn't happen every time when manually running the test, but every
third or forth time.
Rainer
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On Mon, Jan 18, 2021 at 07:10:07AM +0100, Sebastian Huber wrote:
> Hello Jakub,
>
> On 17/01/2021 17:04, Jakub Jelinek via Gcc-patches wrote:
>
> > On Sun, Jan 17, 2021 at 04:25:24PM +0100, Andreas Schwab wrote:
> > > On Jan 17 2021, Jakub Jelinek via Gcc-patches wrote:
> > >
> > > > Kwok, I guess you can reproduce it even on Linux with
> > > > --disable-linux-futex
> > > And all targets that are not explicitly configured in
> > > libcomp/configure.tgt, where --enable-linux-futex is a no-op.
> > Completely untested patch (except for the linux futex version; and RTEMS
> > stuff is missing; I think it doesn't have a function for it but has a
> > counter in the struct, so perhaps fetch it manually from there), it is
> > Sunday, don't want to do more tonight:
>
> here is the RTEMS part:
Ok for trunk with ChangeLog entry, thanks.
I have now committed this after bootstrapping/regtesting on x86_64-linux and
i686-linux and additionally building on x86_64-linux with
--disable-linux-futex and testing libgomp there.
2021-01-18 Jakub Jelinek
* config/linux/sem.h (gomp_sem_getcount): New function.
* config/posix/sem.h (gomp_sem_getcount): New function.
* config/posix/sem.c (gomp_sem_getcount): New function.
* config/accel/sem.h (gomp_sem_getcount): New function.
* task.c (task_fulfilled_p): Use gomp_sem_getcount.
(omp_fulfill_event): Likewise.
--- libgomp/config/linux/sem.h.jj 2021-01-04 10:25:56.160037625 +0100
+++ libgomp/config/linux/sem.h 2021-01-17 16:49:39.900750416 +0100
@@ -85,4 +85,13 @@ gomp_sem_post (gomp_sem_t *sem)
if (__builtin_expect (count & SEM_WAIT, 0))
gomp_sem_post_slow (sem);
}
+
+static inline int
+gomp_sem_getcount (gomp_sem_t *sem)
+{
+ int count = __atomic_load_n (sem, MEMMODEL_RELAXED);
+ if ((count & SEM_WAIT) != 0)
+return -1;
+ return count / SEM_INC;
+}
#endif /* GOMP_SEM_H */
--- libgomp/config/posix/sem.h.jj 2021-01-04 10:25:56.166037557 +0100
+++ libgomp/config/posix/sem.h 2021-01-17 16:49:53.605593659 +0100
@@ -64,6 +64,8 @@ extern void gomp_sem_post (gomp_sem_t *s
extern void gomp_sem_destroy (gomp_sem_t *sem);
+extern int gomp_sem_getcount (gomp_sem_t *sem);
+
#else /* HAVE_BROKEN_POSIX_SEMAPHORES */
typedef sem_t gomp_sem_t;
@@ -84,5 +86,13 @@ static inline void gomp_sem_destroy (gom
{
sem_destroy (sem);
}
+
+static inline int gomp_sem_getcount (gomp_sem_t *sem)
+{
+ int val;
+ if (sem_getvalue (sem, &val) < 0)
+return -1;
+ return val;
+}
#endif /* doesn't HAVE_BROKEN_POSIX_SEMAPHORES */
#endif /* GOMP_SEM_H */
--- libgomp/config/posix/sem.c.jj 2021-01-04 10:25:56.184037354 +0100
+++ libgomp/config/posix/sem.c 2021-01-17 16:52:00.207145847 +0100
@@ -112,6 +112,26 @@ void gomp_sem_destroy (gomp_sem_t *sem)
return;
}
+
+int gomp_sem_getcount (gomp_sem_t *sem)
+{
+ int ret, count;
+
+ ret = pthread_mutex_lock (&sem->mutex);
+ if (ret)
+return -1;
+
+ count = sem->value;
+
+ ret = pthread_mutex_unlock (&sem->mutex);
+ if (ret)
+return -1;
+
+ if (count < 0)
+return -1;
+
+ return count;
+}
#else /* HAVE_BROKEN_POSIX_SEMAPHORES */
void
gomp_sem_wait (gomp_sem_t *sem)
--- libgomp/config/accel/sem.h.jj 2021-01-04 10:25:56.261036482 +0100
+++ libgomp/config/accel/sem.h 2021-01-17 16:53:13.381309036 +0100
@@ -62,4 +62,13 @@ gomp_sem_post (gomp_sem_t *sem)
{
(void) __atomic_add_fetch (sem, 1, MEMMODEL_RELEASE);
}
+
+static inline int
+gomp_sem_getcount (gomp_sem_t *sem)
+{
+ int count = __atomic_load_n (sem, MEMMODEL_RELAXED);
+ if (count < 0)
+return -1;
+ return count;
+}
#endif /* GOMP_SEM_H */
--- libgomp/task.c.jj 2021-01-16 22:52:33.749412323 +0100
+++ libgomp/task.c 2021-01-17 16:54:54.315154777 +0100
@@ -330,7 +330,7 @@ gomp_task_handle_depend (struct gomp_tas
static bool
task_fulfilled_p (struct gomp_task *task)
{
- return __atomic_load_n (&task->completion_sem, __ATOMIC_RELAXED);
+ return gomp_sem_getcount (&task->completion_sem) > 0;
}
/* Called when encountering an explicit task directive. If IF_CLAUSE is
@@ -2406,7 +2406,7 @@ omp_fulfill_event (omp_event_handle_t ev
struct gomp_thread *thr = gomp_thread ();
struct gomp_team *team = thr ? thr->ts.team : NULL;
- if (__atomic_load_n (sem, __ATOMIC_RELAXED))
+ if (gomp_sem_getcount (sem) > 0)
gomp_fatal ("omp_fulfill_event: %p event already fulfilled!\n", sem);
gomp_debug (0, "omp_fulfill_event: %p\n", sem);
Jakub
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
Hello Jakub,
On 17/01/2021 17:04, Jakub Jelinek via Gcc-patches wrote:
On Sun, Jan 17, 2021 at 04:25:24PM +0100, Andreas Schwab wrote:
On Jan 17 2021, Jakub Jelinek via Gcc-patches wrote:
Kwok, I guess you can reproduce it even on Linux with --disable-linux-futex
And all targets that are not explicitly configured in
libcomp/configure.tgt, where --enable-linux-futex is a no-op.
Completely untested patch (except for the linux futex version; and RTEMS
stuff is missing; I think it doesn't have a function for it but has a
counter in the struct, so perhaps fetch it manually from there), it is
Sunday, don't want to do more tonight:
here is the RTEMS part:
diff --git a/libgomp/config/rtems/sem.h b/libgomp/config/rtems/sem.h
index 50b650ab807..0cd74153b05 100644
--- a/libgomp/config/rtems/sem.h
+++ b/libgomp/config/rtems/sem.h
@@ -47,6 +47,11 @@ static inline void gomp_sem_post (gomp_sem_t *sem)
_Semaphore_Post (sem);
}
+static inline int gomp_sem_getcount (gomp_sem_t *sem)
+{
+ return (int) __atomic_load_n (&sem->_count, MEMMODEL_RELAXED);
+}
+
static inline void gomp_sem_destroy (gomp_sem_t *sem)
{
_Semaphore_Destroy (sem);
--
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.hu...@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax: +49-89-18 94 741 - 08
Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On Sun, Jan 17, 2021 at 04:25:24PM +0100, Andreas Schwab wrote:
> On Jan 17 2021, Jakub Jelinek via Gcc-patches wrote:
>
> > Kwok, I guess you can reproduce it even on Linux with --disable-linux-futex
>
> And all targets that are not explicitly configured in
> libcomp/configure.tgt, where --enable-linux-futex is a no-op.
Completely untested patch (except for the linux futex version; and RTEMS
stuff is missing; I think it doesn't have a function for it but has a
counter in the struct, so perhaps fetch it manually from there), it is
Sunday, don't want to do more tonight:
--- libgomp/config/linux/sem.h.jj 2021-01-04 10:25:56.160037625 +0100
+++ libgomp/config/linux/sem.h 2021-01-17 16:49:39.900750416 +0100
@@ -85,4 +85,13 @@ gomp_sem_post (gomp_sem_t *sem)
if (__builtin_expect (count & SEM_WAIT, 0))
gomp_sem_post_slow (sem);
}
+
+static inline int
+gomp_sem_getcount (gomp_sem_t *sem)
+{
+ int count = __atomic_load_n (sem, MEMMODEL_RELAXED);
+ if ((count & SEM_WAIT) != 0)
+return -1;
+ return count / SEM_INC;
+}
#endif /* GOMP_SEM_H */
--- libgomp/config/posix/sem.h.jj 2021-01-04 10:25:56.166037557 +0100
+++ libgomp/config/posix/sem.h 2021-01-17 16:49:53.605593659 +0100
@@ -64,6 +64,8 @@ extern void gomp_sem_post (gomp_sem_t *s
extern void gomp_sem_destroy (gomp_sem_t *sem);
+extern int gomp_sem_getcount (gomp_sem_t *sem);
+
#else /* HAVE_BROKEN_POSIX_SEMAPHORES */
typedef sem_t gomp_sem_t;
@@ -84,5 +86,13 @@ static inline void gomp_sem_destroy (gom
{
sem_destroy (sem);
}
+
+static inline int gomp_sem_getcount (gomp_sem_t *sem)
+{
+ int val;
+ if (sem_getvalue (sem, &val) < 0)
+return -1;
+ return val;
+}
#endif /* doesn't HAVE_BROKEN_POSIX_SEMAPHORES */
#endif /* GOMP_SEM_H */
--- libgomp/config/posix/sem.c.jj 2021-01-04 10:25:56.184037354 +0100
+++ libgomp/config/posix/sem.c 2021-01-17 16:52:00.207145847 +0100
@@ -112,6 +112,26 @@ void gomp_sem_destroy (gomp_sem_t *sem)
return;
}
+
+int gomp_sem_getcount (gomp_sem_t *sem)
+{
+ int ret, count;
+
+ ret = pthread_mutex_lock (&sem->mutex);
+ if (ret)
+return -1;
+
+ count = sem->value;
+
+ ret = pthread_mutex_unlock (&sem->mutex);
+ if (ret)
+return -1;
+
+ if (count < 0)
+return -1;
+
+ return count;
+}
#else /* HAVE_BROKEN_POSIX_SEMAPHORES */
void
gomp_sem_wait (gomp_sem_t *sem)
--- libgomp/config/accel/sem.h.jj 2021-01-04 10:25:56.261036482 +0100
+++ libgomp/config/accel/sem.h 2021-01-17 16:53:13.381309036 +0100
@@ -62,4 +62,13 @@ gomp_sem_post (gomp_sem_t *sem)
{
(void) __atomic_add_fetch (sem, 1, MEMMODEL_RELEASE);
}
+
+static inline int
+gomp_sem_getcount (gomp_sem_t *sem)
+{
+ int count = __atomic_load_n (sem, MEMMODEL_RELAXED);
+ if (count < 0)
+return -1;
+ return count;
+}
#endif /* GOMP_SEM_H */
--- libgomp/task.c.jj 2021-01-16 22:52:33.749412323 +0100
+++ libgomp/task.c 2021-01-17 16:54:54.315154777 +0100
@@ -330,7 +330,7 @@ gomp_task_handle_depend (struct gomp_tas
static bool
task_fulfilled_p (struct gomp_task *task)
{
- return __atomic_load_n (&task->completion_sem, __ATOMIC_RELAXED);
+ return gomp_sem_getcount (&task->completion_sem) > 0;
}
/* Called when encountering an explicit task directive. If IF_CLAUSE is
@@ -2406,7 +2406,7 @@ omp_fulfill_event (omp_event_handle_t ev
struct gomp_thread *thr = gomp_thread ();
struct gomp_team *team = thr ? thr->ts.team : NULL;
- if (__atomic_load_n (sem, __ATOMIC_RELAXED))
+ if (gomp_sem_getcount (sem) > 0)
gomp_fatal ("omp_fulfill_event: %p event already fulfilled!\n", sem);
gomp_debug (0, "omp_fulfill_event: %p\n", sem);
Jakub
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On Jan 17 2021, Jakub Jelinek via Gcc-patches wrote: > Kwok, I guess you can reproduce it even on Linux with --disable-linux-futex And all targets that are not explicitly configured in libcomp/configure.tgt, where --enable-linux-futex is a no-op. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
Hi Jakub,
> On Sun, Jan 17, 2021 at 12:26:26PM +0100, Rainer Orth wrote:
>> >> 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.
>> >
>> > LGTM, thanks.
>>
>> this patch broke Solaris bootstrap, but probably all non-Linux targets:
>>
>> /vol/gcc/src/hg/master/local/libgomp/task.c: In function 'task_fulfilled_p':
>> /vol/gcc/src/hg/master/local/libgomp/task.c:334:1: error: control reaches
>> end of non-void function [-Werror=return-type]
>> 334 | }
>> | ^
>>
>> task_fulfilled_p is
>>
>> return __atomic_load_n (&task->completion_sem, __ATOMIC_RELAXED);
>>
>> but in config/posix/sem.h gomp_sem_t is (for
>> !HAVE_BROKEN_POSIX_SEMAPHORES):
>>
>> typedef sem_t gomp_sem_t;
>>
>> and sem_t being a struct in Solaris .
>
> Oops.
> I guess we want to add to sem.h some API to query current value of the
> semaphore, which could be atomic load for the config/{linux,accel}/sem.h,
> sem_getvalue for config/posix/sem.h (does Solaris implement that?)
it does: this was already in POSIX.1-2001 and even Solaris 11.3 supports XPG6.
Rainer
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On Sun, Jan 17, 2021 at 12:26:26PM +0100, Rainer Orth wrote:
> >> 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.
> >
> > LGTM, thanks.
>
> this patch broke Solaris bootstrap, but probably all non-Linux targets:
>
> /vol/gcc/src/hg/master/local/libgomp/task.c: In function 'task_fulfilled_p':
> /vol/gcc/src/hg/master/local/libgomp/task.c:334:1: error: control reaches end
> of non-void function [-Werror=return-type]
> 334 | }
> | ^
>
> task_fulfilled_p is
>
> return __atomic_load_n (&task->completion_sem, __ATOMIC_RELAXED);
>
> but in config/posix/sem.h gomp_sem_t is (for
> !HAVE_BROKEN_POSIX_SEMAPHORES):
>
> typedef sem_t gomp_sem_t;
>
> and sem_t being a struct in Solaris .
Oops.
I guess we want to add to sem.h some API to query current value of the
semaphore, which could be atomic load for the config/{linux,accel}/sem.h,
sem_getvalue for config/posix/sem.h (does Solaris implement that?)
and dunno what for config/rtems/sem.h.
Kwok, I guess you can reproduce it even on Linux with --disable-linux-futex
Jakub
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
Hi Jakub, >> 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. > > LGTM, thanks. this patch broke Solaris bootstrap, but probably all non-Linux targets: /vol/gcc/src/hg/master/local/libgomp/task.c: In function 'task_fulfilled_p': /vol/gcc/src/hg/master/local/libgomp/task.c:334:1: error: control reaches end of non-void function [-Werror=return-type] 334 | } | ^ task_fulfilled_p is return __atomic_load_n (&task->completion_sem, __ATOMIC_RELAXED); but in config/posix/sem.h gomp_sem_t is (for !HAVE_BROKEN_POSIX_SEMAPHORES): typedef sem_t gomp_sem_t; and sem_t being a struct in Solaris . Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On Sat, Jan 16, 2021 at 07:19:51PM +, Kwok Cheung Yeung wrote:
> > 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?
Ok. As detach clause is at most one, the loop is acceptable, but we
certainly would want to avoid O(n^2) complexities in number of clauses.
> 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 :-).
Yeah, that should DTRT.
>
> > > + #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...
Helping translators.
They already have the "too many %qs clauses" string to translate (and many
have translated it already), the detach word shouldn't be translated, and we
don't want them to translate
"too many % clauses"
"too many % clauses"
"too many % clauses"
...
>
> 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.
LGTM, thanks.
Jakub
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
Thanks for the review.
On 16/01/2021 9:25 am, Jakub Jelinek wrote:
On Fri, Jan 15, 2021 at 03:07:56PM +, 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 % 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
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 % clause in a "
- "task construct");
+ "too many % 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 % clause in a "
- "task construct");
+
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On Fri, Jan 15, 2021 at 03:07:56PM +, 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 % 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 000..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
> +
> +void f (omp_event_handle_t x, omp_event_handle_t y, int z)
> +{
> + #pragma omp task detach (x)
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On Fri, Jan 15, 2021 at 04:58:25PM +, Kwok Cheung Yeung wrote: > On 15/01/2021 3:07 pm, Kwok Cheung Yeung wrote: > > I have tested bootstrapping on x86_64 (no offloading) with no issues, > > and running the libgomp testsuite with Nvidia offloading shows no > > regressions. I have also tested all the gomp.exp tests in the main gcc > > testsuite, also with no issues. I am currently still running the full > > testsuite, but do not anticipate any problems. > > > > Okay to commit on trunk, if the full testsuite run does not show any > > regressions? > > Found an issue already :-( - the libgomp include files are not found when > the tests are run via 'make check'. I have now included the relevant parts > of the include files in the tests themselves. Okay for trunk (to be merged > into the main patch)? This incremental patch is ok. I'll try to review the previous patch tomorrow. Jakub
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On 15/01/2021 3:07 pm, Kwok Cheung Yeung wrote:
I have tested bootstrapping on x86_64 (no offloading) with no issues, and
running the libgomp testsuite with Nvidia offloading shows no regressions. I
have also tested all the gomp.exp tests in the main gcc testsuite, also with no
issues. I am currently still running the full testsuite, but do not anticipate
any problems.
Okay to commit on trunk, if the full testsuite run does not show any
regressions?
Found an issue already :-( - the libgomp include files are not found when the
tests are run via 'make check'. I have now included the relevant parts of the
include files in the tests themselves. Okay for trunk (to be merged into the
main patch)?
Thanks
Kwok
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 c7dda82..f50f748 100644
--- a/gcc/testsuite/c-c++-common/gomp/task-detach-1.c
+++ b/gcc/testsuite/c-c++-common/gomp/task-detach-1.c
@@ -1,7 +1,12 @@
/* { dg-do compile } */
/* { dg-options "-fopenmp" } */
-#include
+typedef enum omp_event_handle_t
+{
+ __omp_event_handle_t_max__ = __UINTPTR_MAX__
+} omp_event_handle_t;
+
+extern void omp_fulfill_event (omp_event_handle_t);
void f (omp_event_handle_t x, omp_event_handle_t y, int z)
{
diff --git a/gcc/testsuite/g++.dg/gomp/task-detach-1.C
b/gcc/testsuite/g++.dg/gomp/task-detach-1.C
index 443d3e8..2f0c650 100644
--- a/gcc/testsuite/g++.dg/gomp/task-detach-1.C
+++ b/gcc/testsuite/g++.dg/gomp/task-detach-1.C
@@ -1,7 +1,10 @@
// { dg-do compile }
// { dg-options "-fopenmp" }
-#include
+typedef enum omp_event_handle_t
+{
+ __omp_event_handle_t_max__ = __UINTPTR_MAX__
+} omp_event_handle_t;
template
void func ()
diff --git a/gcc/testsuite/gcc.dg/gomp/task-detach-1.c
b/gcc/testsuite/gcc.dg/gomp/task-detach-1.c
index fa7315e..611044d 100644
--- a/gcc/testsuite/gcc.dg/gomp/task-detach-1.c
+++ b/gcc/testsuite/gcc.dg/gomp/task-detach-1.c
@@ -1,7 +1,12 @@
/* { dg-do compile } */
/* { dg-options "-fopenmp" } */
-#include
+typedef enum omp_event_handle_t
+{
+ __omp_event_handle_t_max__ = __UINTPTR_MAX__
+} omp_event_handle_t;
+
+extern void omp_fulfill_event (omp_event_handle_t);
void f (omp_event_handle_t x)
{
diff --git a/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90
b/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90
index dc51345..114068e 100644
--- a/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90
+++ b/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90
@@ -2,8 +2,10 @@
! { dg-options "-fopenmp" }
program task_detach_1
- use omp_lib
-
+ use iso_c_binding, only: c_intptr_t
+ implicit none
+
+ integer, parameter :: omp_event_handle_kind = c_intptr_t
integer (kind=omp_event_handle_kind) :: x, y
integer :: z
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On 10/12/2020 2:38 pm, Jakub Jelinek wrote:
On Wed, Dec 09, 2020 at 05:37:24PM +, 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).
I have implemented checking for multiple detach clauses and usage with
mergeable. I have included testcases in c-c++-common/gomp/task-detach-1.c and
gfortran.dg/gomp/task-detach-1.f90.
+ 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.
Changed to use cp_parser_id_expression, and added extra logic from
cp_parser_omp_var_list in looking up the decl.
+ 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, "% clause event handle "
+ "has type %qT rather than "
+ "%",
+ 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
void
foo ()
{
T t;
#pragma omp task detach (t)
;
}
template
void
bar ()
{
T t;
#pragma omp task detach (t)
;
}
void
baz ()
{
foo ();
bar (); // Instantiating this should error
}
Moved type checking to finish_omp_clauses, and testcase added at
g++.dg/gomp/task-detach-1.C.
@@ -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.
Done. g++.dg/gomp/task-detach-1.C contains a test for templates.
--- 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-handl
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
Hello
Thanks for the review. Due to the Christmas holidays I have not finished
addressing all these issues yet, but I expect to be done by the end of this
week. Can this patch still make it for GCC 10, as I believe stage 4 is starting
soon?
Thanks
Kwok
On 10/12/2020 2:38 pm, Jakub Jelinek wrote:
On Wed, Dec 09, 2020 at 05:37:24PM +, 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, "% clause event handle "
+ "has type %qT rather than "
+ "%",
+ 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
void
foo ()
{
T t;
#pragma omp task detach (t)
;
}
template
void
bar ()
{
T t;
#pragma omp task detach (t)
;
}
void
baz ()
{
foo ();
bar (); // 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
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On Thu, Dec 10, 2020 at 03:38:40PM +0100, Jakub Jelinek via Gcc-patches wrote: One further thing, the detach clause effectively writes to the var, so unless you mark the clause decl addressable as I've suggested, you should if (omp_shared_to_firstprivate_optimizable_decl_p (decl)) omp_mark_stores (gimplify_omp_ctxp->outer_context, decl); e.g. in gimplify_adjust_omp_clauses OMP_CLAUSE_DETACH: handling, to make sure that outer parallel/task etc. regions don't try to optimize the var from shared to firstprivate through OMP_CLAUSE_SHARED_READONLY flag on OMP_CLAUSE_SHARED. Jakub
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On Thu, Dec 10, 2020 at 03:38:40PM +0100, Jakub Jelinek via Gcc-patches wrote:
> 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.
Asked and Alex said that both should be invalid. Though, if we implement
detach as passing address of the variable to GOMP_task, if we implicitly
add firstprivate clause it would copy the value from before it has been
initialized. One way to handle that would be not add firstprivate clause
next to detach, but treat detach like a firstprivate clause in most places,
and just for the passing pass it specially (let parent of task pass address
of the variable and let the receiving side recieve the value instead,
which would force task_cpyfn, or handle it more like we handle the bounds
of a taskloop - force the omp_eventhandler_t to be the first variable in the
structure and let GOMP_task write the address not just to *detach, but also
to the first element in the structure.
Jakub
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On Wed, Dec 09, 2020 at 05:37:24PM +, 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, "% clause event handle "
> + "has type %qT rather than "
> + "%",
> + 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
void
foo ()
{
T t;
#pragma omp task detach (t)
;
}
template
void
bar ()
{
T t;
#pragma omp task detach (t)
;
}
void
baz ()
{
foo ();
bar (); // 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 (aga
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On 09/12/2020 5:53 pm, Jakub Jelinek wrote: On Wed, Dec 09, 2020 at 05:37:24PM +, Kwok Cheung Yeung wrote: I believe this patch is largely complete now. I have done a bootstrap on x86_64 and run the testsuites with no regressions. I have also run the libgomp testsuite with offloading to Nvidia and AMD GCN devices, also with no regressions. Is this patch okay for trunk (or would it be more appropriate to wait until GCC 11 is branched off)? I think it is desirable for GCC 11, doesn't need to be deferred, and sorry it is taking me so long. I've paged in the standard wording related to this yesterday and hoped I'd look at this, but didn't manage, will try to do that tomorrow or worst case on Friday. No problem :-), and thanks for looking at the patch. Kwok
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
On Wed, Dec 09, 2020 at 05:37:24PM +, Kwok Cheung Yeung wrote: > I believe this patch is largely complete now. I have done a bootstrap on > x86_64 and run the testsuites with no regressions. I have also run the > libgomp testsuite with offloading to Nvidia and AMD GCN devices, also with > no regressions. Is this patch okay for trunk (or would it be more > appropriate to wait until GCC 11 is branched off)? I think it is desirable for GCC 11, doesn't need to be deferred, and sorry it is taking me so long. I've paged in the standard wording related to this yesterday and hoped I'd look at this, but didn't manage, will try to do that tomorrow or worst case on Friday. Jakub
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
Hello
This is a further update of the patch for task detach support.
- The memory for the event is not mapped on the target. This means that if
omp_fulfill_event is called from an 'omp target' section with a target that
does not share memory with the host, the event will not be fulfilled (and a
segfault will probably occur).
I was thinking of something along the lines of:
#pragma omp task detach (event)
{
}
#pragma omp target
{
omp_fulfill_event (event);
}
Would something like this be expected to work? I cannot find many examples of
the detach clause online, and none of them use any offloading constructs.
I have asked on the omp-lang mailing list - this is not expected to work.
- The tasks awaiting event fulfillment currently wait until there are no other
runnable tasks left. A better approach would be to poll (without blocking) the
waiting tasks whenever any task completes, immediately removing any
now-complete tasks and requeuing any dependent tasks.
This has now been implemented. On every iteration of the main loop in
gomp_barrier_handle_tasks, it first checks to see if any tasks in the detach
queue have a fulfilled completion event, and if so it will remove the task and
requeue any dependent tasks.
I have found another problem with the original blocking approach when the tasks
are on offload devices. On Nvidia and GCN, a bar.sync/s_barrier instruction is
issued when gomp_team_barrier_wake is called to synchronise the threads.
However, if some of the barrier threads are stuck waiting for semaphores
associated with completion events, and the fulfillment of those events are in
other tasks waiting to run, then the result is a deadlock as the threads cannot
synchronise without all the semaphores being released.
I have removed the blocking path on gomp_barrier_handle_tasks altogether, and
omp_fulfill_event now directly wakes the barrier threads to process any tasks
that are now complete.
I have also ensured that the event handle specified on the detach clause is
firstprivate by default on enclosing scopes.
I believe this patch is largely complete now. I have done a bootstrap on x86_64
and run the testsuites with no regressions. I have also run the libgomp
testsuite with offloading to Nvidia and AMD GCN devices, also with no
regressions. Is this patch okay for trunk (or would it be more appropriate to
wait until GCC 11 is branched off)?
Thanks
Kwok
commit 3d82db0fc3623e9dc241bed4c4cfd266574d45e7
Author: Kwok Cheung Yeung
Date: Wed Dec 9 09:33:46 2020 -0800
openmp: Add support for the OpenMP 5.0 task detach clause
2020-12-09 Kwok Cheung Yeung
gcc/
* builtin-types.def (BT_PTR_SIZED_INT): New primitive type.
(BT_FN_PSINT_VOID): New function type.
(BT_FN_VOID_OMPFN_PTR_OMPCPYFN_LONG_LONG_BOOL_UINT_PTR_INT): Rename
to...
(BT_FN_VOID_OMPFN_PTR_OMPCPYFN_LONG_LONG_BOOL_UINT_PTR_INT_PSINT):
...this. Add extra argument.
* gimplify.c (gimplify_scan_omp_clauses): Handle OMP_CLAUSE_DETACH.
(gimplify_adjust_omp_clauses): Likewise.
* omp-builtins.def (BUILT_IN_GOMP_TASK): Change function type to
BT_FN_VOID_OMPFN_PTR_OMPCPYFN_LONG_LONG_BOOL_UINT_PTR_INT_PSINT.
(BUILT_IN_GOMP_NEW_EVENT): New.
* omp-expand.c (expand_task_call): Add detach argument when generating
call to GOMP_task.
* omp-low.c (scan_sharing_clauses): Setup data environment for detach
clause.
(lower_detach_clause): New.
(lower_omp_taskreg): Call lower_detach_clause for detach clause. Add
Gimple statements generated for detach clause.
* tree-core.h (enum omp_clause_code): Add OMP_CLAUSE_DETACH.
* tree-pretty-print.c (dump_omp_clause): Handle OMP_CLAUSE_DETACH.
* tree.c (omp_clause_num_ops): Add entry for OMP_CLAUSE_DETACH.
(omp_clause_code_name): Add entry for OMP_CLAUSE_DETACH.
(walk_tree_1): Handle OMP_CLAUSE_DETACH.
* tree.h (OMP_CLAUSE_DETACH_EXPR): New.
gcc/c-family/
* c-pragma.h (pragma_omp_clause): Add PRAGMA_OMP_CLAUSE_DETACH.
Redefine PRAGMA_OACC_CLAUSE_DETACH.
gcc/c/
* c-parser.c (c_parser_omp_clause_detach): New.
(c_parser_omp_all_clauses): Handle PRAGMA_OMP_CLAUSE_DETACH clause.
(OMP_TASK_CLAUSE_MASK): Add mask for PRAGMA_OMP_CLAUSE_DETACH.
* c-typeck.c (c_finish_omp_clauses): Handle PRAGMA_OMP_CLAUSE_DETACH
clause.
gcc/cp/
* parser.c (cp_parser_omp_clause_detach): New.
(cp_parser_omp_all_clauses): Handle PRAGMA_OMP_CLAUSE_DETACH.
(OMP_TASK_CLAUSE_MASK): Add mask for PRAGMA_OMP_CLAUSE_DETACH.
* semantics.c (finish_omp_clauses): Handle OMP_CLAUSE_DETACH clause.
gcc/fortran/
* dump-parse-tree.c (show_omp_clauses): Handle detach clause.
* frontend-passes.c (gfc_code_walker): Walk detach expression.
* gfortran.h (struct
Re: [PATCH] [WIP] openmp: Add OpenMP 5.0 task detach clause support
Hello
This is an updated version of the WIP patch for task detach support. Any
comments would be welcome!
On 11/11/2020 7:06 pm, Kwok Cheung Yeung wrote:
- No error checking at the front-end.
The detach clause is now parsed properly in C, C++ and Fortran, and will raise
an error if the syntax is incorrect or if the event variable is of the wrong type.
- The memory for the event is not mapped on the target. This means that if
omp_fulfill_event is called from an 'omp target' section with a target that does
not share memory with the host, the event will not be fulfilled (and a segfault
will probably occur).
I was thinking of something along the lines of:
#pragma omp task detach (event)
{
}
#pragma omp target
{
omp_fulfill_event (event);
}
Would something like this be expected to work? I cannot find many examples of
the detach clause online, and none of them use any offloading constructs.
- The tasks awaiting event fulfillment currently wait until there are no other
runnable tasks left. A better approach would be to poll (without blocking) the
waiting tasks whenever any task completes, immediately removing any now-complete
tasks and requeuing any dependent tasks.
This has now been implemented. On every iteration of the main loop in
gomp_barrier_handle_tasks, it first checks to see if any tasks in the detach
queue have a fulfilled completion event, and if so it will remove the task and
requeue any dependent tasks.
Thanks
Kwok
From 3611024b39ea5b264ec2fd35ffa64360861052af Mon Sep 17 00:00:00 2001
From: Kwok Cheung Yeung
Date: Fri, 27 Nov 2020 11:59:12 -0800
Subject: [PATCH] openmp: Add support for the OpenMP 5.0 task detach clause
2020-11-27 Kwok Cheung Yeung
gcc/
* builtin-types.def (BT_PTR_SIZED_INT): New primitive type.
(BT_FN_PSINT_VOID): New function type.
(BT_FN_VOID_OMPFN_PTR_OMPCPYFN_LONG_LONG_BOOL_UINT_PTR_INT): Rename
to...
(BT_FN_VOID_OMPFN_PTR_OMPCPYFN_LONG_LONG_BOOL_UINT_PTR_INT_PSINT):
...this. Add extra argument.
* gimplify.c (gimplify_scan_omp_clauses): Handle OMP_CLAUSE_DETACH.
(gimplify_adjust_omp_clauses): Likewise.
* omp-builtins.def (BUILT_IN_GOMP_TASK): Change function type to
BT_FN_VOID_OMPFN_PTR_OMPCPYFN_LONG_LONG_BOOL_UINT_PTR_INT_PSINT.
(BUILT_IN_GOMP_NEW_EVENT): New.
* omp-expand.c (expand_task_call): Add detach argument when generating
call to GOMP_task.
* omp-low.c (scan_sharing_clauses): Setup data environment for detach
clause.
(lower_detach_clause): New.
(lower_omp_taskreg): Call lower_detach_clause for detach clause. Add
Gimple statements generated for detach clause.
* tree-core.h (enum omp_clause_code): Add OMP_CLAUSE_DETACH.
* tree-pretty-print.c (dump_omp_clause): Handle OMP_CLAUSE_DETACH.
* tree.c (omp_clause_num_ops): Add entry for OMP_CLAUSE_DETACH.
(omp_clause_code_name): Add entry for OMP_CLAUSE_DETACH.
(walk_tree_1): Handle OMP_CLAUSE_DETACH.
* tree.h (OMP_CLAUSE_DETACH_EXPR): New.
gcc/c-family/
* c-pragma.h (pragma_omp_clause): Add PRAGMA_OMP_CLAUSE_DETACH.
Redefine PRAGMA_OACC_CLAUSE_DETACH.
gcc/c/
* c-parser.c (c_parser_omp_clause_detach): New.
(c_parser_omp_all_clauses): Handle PRAGMA_OMP_CLAUSE_DETACH clause.
(OMP_TASK_CLAUSE_MASK): Add mask for PRAGMA_OMP_CLAUSE_DETACH.
* c-typeck.c (c_finish_omp_clauses): Handle PRAGMA_OMP_CLAUSE_DETACH
clause.
gcc/cp/
* parser.c (cp_parser_omp_clause_detach): New.
(cp_parser_omp_all_clauses): Handle PRAGMA_OMP_CLAUSE_DETACH.
(OMP_TASK_CLAUSE_MASK): Add mask for PRAGMA_OMP_CLAUSE_DETACH.
* semantics.c (finish_omp_clauses): Handle OMP_CLAUSE_DETACH clause.
gcc/fortran/
* dump-parse-tree.c (show_omp_clauses): Handle detach clause.
* frontend-passes.c (gfc_code_walker): Walk detach expression.
* gfortran.h (struct gfc_omp_clauses): Add detach field.
(gfc_c_intptr_kind): New.
* openmp.c (gfc_free_omp_clauses): Free detach clause.
(gfc_match_omp_detach): New.
(enum omp_mask1): Add OMP_CLAUSE_DETACH.
(enum omp_mask2): Remove OMP_CLAUSE_DETACH.
(gfc_match_omp_clauses): Handle OMP_CLAUSE_DETACH for OpenMP.
(OMP_TASK_CLAUSES): Add OMP_CLAUSE_DETACH.
* trans-openmp.c (gfc_trans_omp_clauses): Handle detach clause.
* trans-types.c (gfc_c_intptr_kind): New.
(gfc_init_kinds): Initialize gfc_c_intptr_kind.
* types.def (BT_PTR_SIZED_INT): New type.
(BT_FN_PSINT_VOID): New function type.
(BT_FN_VOID_OMPFN_PTR_OMPCPYFN_LONG_LONG_BOOL_UINT_PTR_INT): Rename
to...
(BT_FN_VOID_OMPFN_PTR_OMPCPYFN_LONG_LONG_BOOL_UINT_PTR_INT_PSINT):
...this. Add extra argument.
libgomp/
* fortran.c (omp_fulfill_event_): New.
* libgomp.h
