Hi Tobias, Thanks for the review.
On 08/08/2026 19:45, Tobias Burnus wrote:
Paul-Antoine Arras wrote:When the inscan modifier is present on a for construct, a call to GOMP_loop_end (or one of its variants) is already emitted. The extra call to one of the *_worksharing_end functions, enabled by -fopenmp-ompt, is therefore redundant.Looking at the code and the dump, I was confused - and I know now why :-) With -fopenmp-ompt, the code was (before this patch): D.3286 = __builtin_GOMP_loop_static_worksharing_start (100); ... __builtin_GOMP_loop_end_nowait (); __builtin_GOMP_loop_static_worksharing_end (); The 'GOMP_loop_static_worksharing_start' dispatches the ompt_work_loop_static callback with ompt_scope_begin And both 'GOMP_loop_end_nowait' and 'GOMP_loop_static_worksharing_end' dispatch: 'ompt_work_loop_static' with ompt_scope_end. [Side remark: the nowait function knows the schedule type, i.e. it might also dispatch other callbacks; but for static both are identical.] Thus, the patch now removes the call to the latter. This now LGTM. However, without -fopenmp-ompt, the code looks as follows: D.3282 = __builtin_GOMP_loop_static_worksharing (100); __builtin_GOMP_loop_end_nowait (); Both invoke the ompt_work_loop_static callback - the latter, see above, with ompt_scope_end. However, the former with ompt_scope_beginend. - That's inconsistent! If there is an 'end', there needs to be a 'begin' not a 'beginend'! Hence: If there is latere a call to GOMP_loop_end_nowait, the first call must be to GOMP_loop_static_worksharing_start. * * *gcc/ChangeLog: * omp-expand.cc (expand_omp_for_static_nochunk): Do not emit *_static_worksharing_end call with inscan modifier.Spurious '*' at the front of the second line. * * *--- a/gcc/omp-expand.cc +++ b/gcc/omp-expand.cc @@ -5610,7 +5610,10 @@ expand_omp_for_static_nochunk (struct omp_region *region, }gsi = gsi_last_nondebug_bb (exit_bb);- if (flag_openmp_ompt) + if (flag_openmp_ompt + && !(fd->have_reductemp + || ((fd->have_pointer_condtemp || fd->have_scantemp) + && !fd->have_nonctrl_scantemp))) { /* Insert call to GOMP_*_static_worksharing_end at the end of exit_bb. */ @@ -6367,7 +6370,7 @@ expand_omp_for_static_chunk (struct omp_region *region, }gsi = gsi_last_nondebug_bb (exit_bb);- if (flag_openmp_ompt) + if (flag_openmp_ompt && !(fd->have_reductemp || fd->have_pointer_condtemp)) { /* Insert call to GOMP_*_static_worksharing_end at the end of exit_bb. */In both functions: * If there will be a GOMP_loop_end_nowait call, the *static*_start call has to be called. [Also without -fopenmp-ompt], otherwise only with -fopenmp-ompt. Any maybe also add a comment that - as the call to GOMP_loop_end_nowait implies the end of the scope, '_start' has to be used and '_end' can be skipped. Maybe, it looks cleaner with some common boolean to handle the check both for the adding of _start and (with a !boolean) for (not) adding the _end call? * * * In libgomp/loop.c, can you add a comment to the non-"_start" function that the "_start" one is called if there is a "_end_nowait" call - and note at the "_start" function that no _end" is expected if there is an _end_nowait" call? – Or something like that?
Please find attached an updated patch. Cheers, -- PA
From 42238c860458c7355cddab45b730ceee78c51d73 Mon Sep 17 00:00:00 2001 From: Paul-Antoine Arras <[email protected]> Date: Fri, 7 Aug 2026 15:33:37 +0200 Subject: [PATCH v2] openmp: Adjust calls to OMPT variants depending on loop-end call When the inscan modifier is present on a for construct, a call to GOMP_loop_end (or one of its variants) is already emitted. The extra call to one of the *_worksharing_end functions, enabled by -fopenmp-ompt, is therefore redundant. Furthermore, even without -fopenmp-ompt, the inscan modifier implies a call to GOMP_loop_end which requires the start variant of GOMP_loop_static_worksharing. gcc/ChangeLog: * omp-expand.cc (expand_omp_for_static_nochunk, expand_omp_for_static_chunk): Adjust calls to OMPT variants with GOMP_loop_end. gcc/testsuite/ChangeLog: * c-c++-common/gomp/scan-8.c: New test. * c-c++-common/gomp/scan-9.c: New test. libgomp/ChangeLog: * loop.c (GOMP_loop_static_worksharing_start, GOMP_loop_static_worksharing_end): Update comments. --- gcc/omp-expand.cc | 34 +++++++++++++++++------- gcc/testsuite/c-c++-common/gomp/scan-8.c | 22 +++++++++++++++ gcc/testsuite/c-c++-common/gomp/scan-9.c | 21 +++++++++++++++ libgomp/loop.c | 6 +++-- 4 files changed, 71 insertions(+), 12 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/gomp/scan-8.c create mode 100644 gcc/testsuite/c-c++-common/gomp/scan-9.c diff --git a/gcc/omp-expand.cc b/gcc/omp-expand.cc index 0d801e0d68a..65f65da4b07 100644 --- a/gcc/omp-expand.cc +++ b/gcc/omp-expand.cc @@ -5225,6 +5225,12 @@ expand_omp_for_static_nochunk (struct omp_region *region, t = fold_convert (itype, t); n = force_gimple_operand_gsi (&gsi, t, true, NULL_TREE, true, GSI_SAME_STMT); + /* When GOMP_loop_end (or one of its variants) is emitted (e.g. with the + inscan modifier), which already implies the end of the scope, _start + variants of GOMP builtin calls have to be used and _end can be skipped. */ + bool has_gomp_loop_end = fd->have_reductemp + || ((fd->have_pointer_condtemp || fd->have_scantemp) + && !fd->have_nonctrl_scantemp); { /* Fetch the thread/team id and the number of threads/teams in a single call to GOMP_loop_static_worksharing or @@ -5239,13 +5245,15 @@ expand_omp_for_static_nochunk (struct omp_region *region, { case GF_OMP_FOR_KIND_FOR: decl = builtin_decl_explicit ( - flag_openmp_ompt ? BUILT_IN_GOMP_LOOP_STATIC_WORKSHARING_START - : BUILT_IN_GOMP_LOOP_STATIC_WORKSHARING); + flag_openmp_ompt || has_gomp_loop_end + ? BUILT_IN_GOMP_LOOP_STATIC_WORKSHARING_START + : BUILT_IN_GOMP_LOOP_STATIC_WORKSHARING); break; case GF_OMP_FOR_KIND_DISTRIBUTE: decl = builtin_decl_explicit ( - flag_openmp_ompt ? BUILT_IN_GOMP_DISTRIBUTE_STATIC_WORKSHARING_START - : BUILT_IN_GOMP_DISTRIBUTE_STATIC_WORKSHARING); + flag_openmp_ompt || has_gomp_loop_end + ? BUILT_IN_GOMP_DISTRIBUTE_STATIC_WORKSHARING_START + : BUILT_IN_GOMP_DISTRIBUTE_STATIC_WORKSHARING); break; default: gcc_unreachable (); @@ -5610,7 +5618,7 @@ expand_omp_for_static_nochunk (struct omp_region *region, } gsi = gsi_last_nondebug_bb (exit_bb); - if (flag_openmp_ompt) + if (flag_openmp_ompt && !has_gomp_loop_end) { /* Insert call to GOMP_*_static_worksharing_end at the end of exit_bb. */ @@ -6056,6 +6064,10 @@ expand_omp_for_static_chunk (struct omp_region *region, n = force_gimple_operand_gsi (&gsi, t, true, NULL_TREE, true, GSI_SAME_STMT); + /* When GOMP_loop_end (or one of its variants) is emitted (e.g. with the + inscan modifier), which already implies the end of the scope, _start + variants of GOMP builtin calls have to be used and _end can be skipped. */ + bool has_gomp_loop_end = fd->have_reductemp || fd->have_pointer_condtemp; { /* Fetch the thread/team id and the number of threads/teams in a single call to GOMP_loop_static_worksharing or @@ -6070,13 +6082,15 @@ expand_omp_for_static_chunk (struct omp_region *region, { case GF_OMP_FOR_KIND_FOR: decl = builtin_decl_explicit ( - flag_openmp_ompt ? BUILT_IN_GOMP_LOOP_STATIC_WORKSHARING_START - : BUILT_IN_GOMP_LOOP_STATIC_WORKSHARING); + flag_openmp_ompt || has_gomp_loop_end + ? BUILT_IN_GOMP_LOOP_STATIC_WORKSHARING_START + : BUILT_IN_GOMP_LOOP_STATIC_WORKSHARING); break; case GF_OMP_FOR_KIND_DISTRIBUTE: decl = builtin_decl_explicit ( - flag_openmp_ompt ? BUILT_IN_GOMP_DISTRIBUTE_STATIC_WORKSHARING_START - : BUILT_IN_GOMP_DISTRIBUTE_STATIC_WORKSHARING); + flag_openmp_ompt || has_gomp_loop_end + ? BUILT_IN_GOMP_DISTRIBUTE_STATIC_WORKSHARING_START + : BUILT_IN_GOMP_DISTRIBUTE_STATIC_WORKSHARING); break; default: gcc_unreachable (); @@ -6367,7 +6381,7 @@ expand_omp_for_static_chunk (struct omp_region *region, } gsi = gsi_last_nondebug_bb (exit_bb); - if (flag_openmp_ompt) + if (flag_openmp_ompt && !has_gomp_loop_end) { /* Insert call to GOMP_*_static_worksharing_end at the end of exit_bb. */ diff --git a/gcc/testsuite/c-c++-common/gomp/scan-8.c b/gcc/testsuite/c-c++-common/gomp/scan-8.c new file mode 100644 index 00000000000..cff8c03b349 --- /dev/null +++ b/gcc/testsuite/c-c++-common/gomp/scan-8.c @@ -0,0 +1,22 @@ +/* { dg-do compile } */ +/* { dg-additional-options "-fopenmp-ompt -fdump-tree-ompexp" } */ + +/* Check that an extra, OMPT variant of GOMP_loop_end is not emitted when the + inscan modifier is present on the for construct. */ + +#define N 100 + +void f(void) { + int a[N], b[N]; + int x = 0; + +#pragma omp parallel for simd reduction(inscan, +: x) + for (int k = 0; k < N; k++) { + x += a[k]; +#pragma omp scan inclusive(x) + b[k] = x; + } +} + +/* { dg-final { scan-tree-dump "__builtin_GOMP_loop_end_nowait \\(" "ompexp" } } */ +/* { dg-final { scan-tree-dump-not "__builtin_GOMP_loop_end_nowait \\(\\);\[\t\n \]*__builtin_GOMP_loop_static_worksharing_end \\(\\);" "ompexp" } } */ diff --git a/gcc/testsuite/c-c++-common/gomp/scan-9.c b/gcc/testsuite/c-c++-common/gomp/scan-9.c new file mode 100644 index 00000000000..f0335536339 --- /dev/null +++ b/gcc/testsuite/c-c++-common/gomp/scan-9.c @@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-additional-options "-fdump-tree-ompexp" } */ + +/* Check that the start variant of GOMP_loop_static_worksharing is emitted + when the inscan modifier is present on the for construct. */ + +#define N 100 + +void f(void) { + int a[N], b[N]; + int x = 0; + +#pragma omp parallel for simd reduction(inscan, +: x) + for (int k = 0; k < N; k++) { + x += a[k]; +#pragma omp scan inclusive(x) + b[k] = x; + } +} + +/* { dg-final { scan-tree-dump "__builtin_GOMP_loop_static_worksharing_start \\(" "ompexp" } } */ diff --git a/libgomp/loop.c b/libgomp/loop.c index 0692663215f..7474279fcd6 100644 --- a/libgomp/loop.c +++ b/libgomp/loop.c @@ -1201,7 +1201,8 @@ GOMP_loop_static_worksharing (unsigned long long niter return nthreads + tid * 1I; } -/* OMPT variant enabled by -fopenmp-ompt. */ +/* OMPT variant enabled by -fopenmp-ompt and when GOMP_loop_end is called + (e.g. with the inscan modifier). */ _Complex int GOMP_loop_static_worksharing_start (unsigned long long niter @@ -1224,7 +1225,8 @@ GOMP_loop_static_worksharing_dispatch (unsigned long long start __attribute__ ((unused))) {} -/* Stub for OMPT callback enabled by -fopenmp-ompt. */ +/* Stub for OMPT callback enabled by -fopenmp-ompt, except when GOMP_loop_end is + already called (e.g. with the inscan modifier). */ void GOMP_loop_static_worksharing_end (void) -- 2.53.0
