Fortran: Add OpenMP's assume(s) directives

  Hi Jakub,

On 04.10.22 12:19, Jakub Jelinek wrote:

On Sun, Oct 02, 2022 at 07:47:18PM +0200, Tobias Burnus wrote:


@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?

 * * *

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.

+      if (is_omp_declarative_stmt (st) || is_omp_informational_stmt (st))
+       {
+         gfc_error ("Invalid %qs directive at %L in %s clause: declarative, "
+                    "informational and meta directives not permitted",
+                    gfc_ascii_statement (st, true), &old_loc,
+                    is_absent ? "ABSENT" : "CONTAINS");


Do you think we should do the same for C/C++?
Right now it doesn't differentiate between invalid directive names and
names of declarative, informational or meta directives.

Maybe - it might help users to understand why something went wrong, on the other hand, I do not really think that a user adds 'absent(declare variant)', but I might be wrong.

+           = (gfc_statement *) xrealloc ((*assume)->absent,
+                                         sizeof (gfc_statement)
+                                         * (*assume)->n_absent);


XRESIZEVEC?

Aha, that's the macro name!


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)
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

And the simplest way to do the merge seems to use gfc_match_omp_clauses,
which already handles merging  'absent(target) absent(teams)'.

Thus, I pre-populate the clause list with the assumption values from
the previous directive.

Additionally, there shouldn't be a leak as inside gfc_omp_match_clauses is:
      gfc_free_omp_clauses (c);
      return MATCH_ERROR;
which frees the memory. To avoid double freeing, a possibly pre-existing
'gfc_current_ns->omp_assumes' has to be set to NULL.

The other question is whether the spec is clear, e.g. is the following valid?
  !$omp assumes no_openmp
  !$omp assumes no_openmp
In each directive, no_openmp is unique but the combination is not (but it
should be fine, here). While for
  !$omp assumes absent(target)
  !$omp assumes contains(target)
there is surely an issue.



+  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?

This turns O(n^2) into O(n) and I am pretty sure in the common
case, it is much faster than using a hash or bitmap.

Reason: There 38 permitted directives of which 7 are rejected at parse time,
hence 31 remain. The worst case is to have as input:
 dir_1, dir_2, ..., dir_31, dir_31,... dir_31
Thus, there are '(n-1) + (n-2) + ... + (n-30) + 1' iterations until
the first error is found, which is O(n*3O) = O(n).

In the real world, I assume n <= 5 and it seems to be faster to
do 4+3+2+1 = 10 comparisons rather than starting to construct
a hash or a bitmap.

However, if you think it still makes sense to create a bitmap or hash,
I can do it.

Tobias


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra?e 201, 80634 M?nchen; Gesellschaft mit beschr?nkter Haftung; Gesch?ftsf?hrer: Thomas Heurung, Frank Th?rauf; Sitz der Gesellschaft: M?nchen; Registergericht M?nchen, HRB 106955
-------------- next part --------------
A non-text attachment was scrubbed...
Name: update-fopenmp-simd.diff
Type: text/x-patch
Size: 4610 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20221004/04c2f69e/attachment.bin>
  

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
  

Hi Jakub,

On 04.10.22 14:58, Jakub Jelinek via Gcc-patches wrote:

On Tue, Oct 04, 2022 at 02:26:13PM +0200, Tobias Burnus wrote:


On Sun, Oct 02, 2022 at 07:47:18PM +0200, Tobias Burnus wrote:
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi

OK?


Ok, thanks.

Committed as https://gcc.gnu.org/r13-3063-g8792047470073df0da4a5b91997d6058193d7676

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.

Now added. I noted that I have different kinds/categories than you used in c-family/c-omp.c; and my impression that standalone vs. block vs delimited is a different category than informational/meta/...

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.

...


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.

I used now pow2p_hwi as popcount did not exist (and I didn't want to add an #include or use __builtin_popcount), not that either variant is clearer and it is neither performance critical nor is neither of "(x & -x) == x" and "popcount(x) == 1" slow.

I don't understand the point of preallocation of gfc_omp_clauses here,
...

That's now gone. As I have to check the duplication right after parsing ? but before merging, I can no longer do it during resolution. Instead of keeping track of the directives separately, I now moved the checking to the directive parsing itself.

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.

I am not sure the handling of nested/repeated informational/declarative directives is very clear, but that's a separate issue. (Namely, OpenMP spec issue 3362.)

Updated patch enclosed. And thanks for your comments!

OK?

Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra?e 201, 80634 M?nchen; Gesellschaft mit beschr?nkter Haftung; Gesch?ftsf?hrer: Thomas Heurung, Frank Th?rauf; Sitz der Gesellschaft: M?nchen; Registergericht M?nchen, HRB 106955
-------------- next part --------------
A non-text attachment was scrubbed...
Name: omp-assume-fortran-v2.diff
Type: text/x-patch
Size: 41008 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20221005/f91a5d07/attachment-0001.bin>