Re: [Patch] OpenMP: Add strictly nested API call check [PR102972]

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Oct 29, 2021 at 12:09:55PM +0200, Tobias Burnus wrote:
> The original motivation was to fix the routine part
> of the restriction quoted below. Namely that the only
> routines calls to
>   omp_get_num_teams() and omp_get_team_num()
> are permitted in teams when closely nested.
> 
> 
> "Restrictions to the teams construct are as follows:
> ...
> • distribute regions, including any distribute regions arising from composite constructs,
> parallel regions, including any parallel regions arising from combined constructs, loop
> regions, omp_get_num_teams() regions, and omp_get_team_num() regions are the
> only OpenMP regions that may be strictly nested inside the teams region."
> 
> 
> While being there, I found one issue related to the ancestor
> check – which checked too strictly – and in the generic check
> which assumed that the DECL_NAME in Fortran had the '_' suffix
> while only the assembler name has.
> 
> That worked well with '_' as DECL_NAME then matched the C name
> but for the integer(8) version, only ..._8_ was matched and
> DECL_NAME only contained ..._8 without tailing '_'.
> 
> The assembler name is also needed because in Fortran,
>  module m
>  contains
>    subroutine omp_is_initial_device ()
> has an OpenMP API name in DECL_NAME but internally, it is
> something like m_MOD_omp_is_initial_device_ - which is an
> odd user name but is not the API routine name.

I'm afraid using DECL_ASSEMBLER_NAME opens a new can of worms.
For one, it shouldn't be HAS_DECL_ASSEMBLER_NAME_P guarded, we either want
to use one or the other always, not randomly pick between them depending
on whether a function already got an assembler name or not.
But, for DECL_ASSEMBLER_NAME, I'm afraid one needs to
targetm.strip_name_encoding and also strip user_label_prefix if any.

At least for C++,
namespace A
{
  int omp_is_initial_device () { return 0; }
};
is meant to be checked by
      || (DECL_CONTEXT (fndecl) != NULL_TREE
          && TREE_CODE (DECL_CONTEXT (fndecl)) != TRANSLATION_UNIT_DECL)
If that doesn't work for Fortran modules, we need to find out something
different, e.g. setjmp_or_longjmp_p also relies on that...

On the other side, when we use DECL_NAME we don't currently differentiate
between:
extern "C" int omp_is_initial_device ();
and say
extern int omp_is_initial_device (double, float);
where the latter is in C++ mangled differently.  Sure, one can't use
the latter together with #include <omp.h>...

> --- a/gcc/omp-low.c
> +++ b/gcc/omp-low.c
> @@ -3911,7 +3911,7 @@ setjmp_or_longjmp_p (const_tree fndecl)
>  /* Return true if FNDECL is an omp_* runtime API call.  */
>  
>  static bool
> -omp_runtime_api_call (const_tree fndecl)
> +omp_runtime_api_call (tree fndecl, bool permit_num_teams)
>  {
>    tree declname = DECL_NAME (fndecl);
>    if (!declname
> @@ -3920,6 +3920,8 @@ omp_runtime_api_call (const_tree fndecl)
>        || !TREE_PUBLIC (fndecl))
>      return false;
>  
> +  if (HAS_DECL_ASSEMBLER_NAME_P (fndecl))
> +    declname = DECL_ASSEMBLER_NAME (fndecl);
>    const char *name = IDENTIFIER_POINTER (declname);
>    if (!startswith (name, "omp_"))
>      return false;
> @@ -4029,7 +4031,17 @@ omp_runtime_api_call (const_tree fndecl)
>  		  && (name[4 + len + 1] == '\0'
>  		      || (mode > 1
>  			  && strcmp (name + 4 + len + 1, "8_") == 0)))))
> -	return true;
> +	{
> +	  /* Only omp_get_num_teams + omp_get_team_num.  */
> +	  if (permit_num_teams
> +	      && mode == 1
> +	      && (strncmp (name + 4, "get_num_teams",
> +			   strlen ("get_num_teams")) == 0
> +		  || strncmp (name + 4, "get_team_num",
> +			      strlen ("get_team_num")) == 0))
> +	    return false;
> +	  return true;
> +	}
>      }
>    return false;
>  }

As mentioned in the PR, I really don't like this permit_num_teams argument,
IMHO it is a caller that should check it, otherwise we end up in the
function with myriads of future exceptions etc.
But, if the stripping of the prefixes is non-trivial, perhaps
omp_runtime_api_call shouldn't return bool, but const char *, either NULL
for "this isn't an OpenMP API call", or pointer to the actual name starting
with "omp_", so that callers can check further.

As for tests where you are adding parallel to avoid the new diagnostics,
I'd suggest parallel if(0) instead, no need to create any extra threads...

	Jakub