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

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