Re: [Patch] Fortran: Add OpenMP's assume(s) directives

[Jakub Jelinek <jakub@redhat.com>]

On Tue, Oct 04, 2022 at 02:26:13PM +0200, Tobias Burnus wrote:
> Hi Jakub,
> 
> On 04.10.22 12:19, Jakub Jelinek wrote:
> 
> On Sun, Oct 02, 2022 at 07:47:18PM +0200, Tobias Burnus wrote:
> 
> 
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -2749,9 +2749,9 @@ have support for @option{-pthread}. @option{-fopenmp} implies
> @opindex fopenmp-simd
> @cindex OpenMP SIMD
> @cindex SIMD
> -Enable handling of OpenMP's SIMD directives with @code{#pragma omp}
> -in C/C++ and @code{!$omp} in Fortran. Other OpenMP directives
> -are ignored.
> +Enable handling of OpenMP's SIMD directives and OPENMP's @code{assume} directive
> 
> 
> s/OPENMP/OpenMP/
> 
> We actually handle more directives, @code{declare reduction},
> @code{ordered}, @code{scan}, @code{loop} and combined/composite directives
> with @code{simd} as constituent.
> ...
> And now in C++ we handle also the attribute syntax (guess we should update
> the text for that here as well as in -fopenmp entry).
> 
> Updated suggestion attached – I still need to update the main patch.
> 
> (I also added 'declare simd' to the list. And I updated Fortran for scan + loop.)
> 
> OK?

Ok, thanks.

> Wouldn't this be better table driven (like c_omp_directives
> in c-family, though guess for Fortran you can just use spaces
> in the name, don't need 3 strings for the separate tokens)?
> Because I think absent/contains isn't the only spot where
> you need directive names, metadirective is another.
> 
> Maybe – I think there are already way to many string repetitions. One problem is that metadirectives permit combined/composite constructs while 'assume(s)' does not. I on purpose did not parse them, but probably in light of Metadirectives, I should.
> 
> I will take a look.

It is true that metadirective supports combined/composite constructs,
and so do we in the C++ attribute case, still we IMHO can use the C/C++
table as is.and it doesn't need to include combined/composite constructs.

The thing is that for the metadirective/C++ attribute case, all we need to
know is to discover the directive category (declarative, stand-alone,
construct, informational, ...) and for that it is enough to parse the
first directive-name in combined/composite constructs.  Both metadirectives
and C++ attributes then have the name of the directive followed by clauses
so we effectively have to use the normal parsing of directives/clauses
there (except perhaps not end on end of directive but on unbalanced closing
paren).  And then there is the absent/contains case, where we only
allow non-combined/composite, so there we need to try to match the directive
name from the table and make sure it is followed by , or ).

> But also, resizing each time a single entry is added to the list isn't
> good for compile time, would be nice to grow the allocation size in
> powers of 2 or so.
> 
> I only expect a very small number – and did not want to keep track of yet another number.
> 
> However, the following should work:
> 
> 
>  if (old_n_absent = 0)
>    absent = ... sizeof() * 1
>  else if (popcount (old_n_absent) == 1)
>    absent = ... sizeof() * (old_n_absent) * 2)

Yeah.  Or for 0 allocate say 8 and
use (pow2p_hwi (old_n_absent) && old_n_absent >= 8)
in the else if.

> that allocates: 1, 2, 4, 8, 16, 32, ... without keeping track of the number.
> 
> 
> 
> +gfc_match_omp_assumes (void)
> +{
> +  locus loc = gfc_current_locus;
> +  gfc_omp_clauses *c = gfc_get_omp_clauses ();
> +  c->assume = gfc_current_ns->omp_assumes;
> +  if (!gfc_current_ns->proc_name
> +      || (gfc_current_ns->proc_name->attr.flavor != FL_MODULE
> +         && !gfc_current_ns->proc_name->attr.subroutine
> +         && !gfc_current_ns->proc_name->attr.function))
> +    {
> +      gfc_error ("!OMP ASSUMES at %C must be in the specification part of a "
> +                "subprogram or module");
> +      return MATCH_ERROR;
> +    }
> +  if (gfc_match_omp_clauses (&c, omp_mask (OMP_CLAUSE_ASSUMPTIONS), true, true,
> +                            false, false, false, false) != MATCH_YES)
> +    {
> +      gfc_current_ns->omp_assumes = NULL;
> +      return MATCH_ERROR;
> +    }
> 
> 
> 
> I don't understand the point of preallocation of gfc_omp_clauses here,
> can't it be allocated inside of gfc_match_omp_clauses like everywhere else?
> Because otherwise it e.g. leaks if the first error is reported.
> 
> This is supposed to handle:
>  subroutine foo()
>    !$omp assumes absent(target)
>    !$omp assumes absent(teams)
>  end
> 
> I did not spot anything which states that it is invalid.
> (I might have missed it, however.) And if it is valid,
> I assume it is equivalent to:
> 
>  subroutine foo()
>    !$omp assumes absent(target, teams)
>  end

It is not equivalent to that, because while we have the restriction
that the same list item can't appear multiple times on the same directive,
it can appear multiple times on multiple directives.
So,
  subroutine foo()
    !$omp assumes absent(target, target)
  end
or
  subroutine foo()
    !$omp assumes absent(target) absent(target)
  end
etc. are invalid but
  subroutine foo()
    !$omp assumes absent(target)
    !$omp assumes absent(target)
  end
is fine (sure, redundant).

> +  for (int i = 0; i < assume->n_contains; i++)
> +    for (int j = i + 1; j < assume->n_contains; j++)
> +      if (assume->contains[i] == assume->contains[j])
> +       gfc_error ("%qs directive mentioned multiple times in %s clause in %s "
> +                  "directive at %L",
> +                  gfc_ascii_statement (assume->contains[i], true),
> +                  "CONTAINS", directive, loc);
> 
> 
> 
> This is O(n^2)?  Can't you use a bitmap or hash map instead?
> 
> How about adding a 'break; after the all the gfc_error instead?

True, I guess I can live with that.  It is less user-friendly
because it will print just one of the errors rather than all of them,
though typically one will not have too many repetitions in there and
can fix them one at a time as well.

	Jakub