Re: [PATCH] openmp: Do not emit OMPT loop-end call with inscan

[Tobias Burnus]

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?

Thanks,

Tobias