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

[Tobias Burnus <tobias@codesourcery.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6818 bytes --]

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?

 * * *

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

[-- Attachment #2: update-fopenmp-simd.diff --]
[-- Type: text/x-patch, Size: 4610 bytes --]

OpenMP: Update invoke.texi and fix fortran/parse.cc for -fopenmp-simd

Split off from the 'Fortran: Add OpenMP's assume(s) directives' patch.

gcc/
	* doc/invoke.texi (-fopenmp): Mention C++ attribut syntax.
	(-fopenmp-simd): Likewise; update permitted directives.

gcc/fortran/
	* parse.cc (decode_omp_directive): Handle '(end) loop' and 'scan'
	also with -fopenmp-simd.

gcc/testsuite/
	* gfortran.dg/gomp/openmp-simd-7.f90: New test.

 gcc/doc/invoke.texi                              | 12 ++++++++----
 gcc/fortran/parse.cc                             |  6 +++---
 gcc/testsuite/gfortran.dg/gomp/openmp-simd-7.f90 | 23 +++++++++++++++++++++++
 3 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index a5dc6377835..e0c2c57c9b2 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -2737,7 +2737,8 @@ can be omitted, to use a target-specific default value.
 @item -fopenmp
 @opindex fopenmp
 @cindex OpenMP parallel
-Enable handling of OpenMP directives @code{#pragma omp} in C/C++ and
+Enable handling of OpenMP directives @code{#pragma omp} in C/C++,
+@code{[[omp::directive(...)]]} and @code{[[omp::sequence(...)]]} in C++ and
 @code{!$omp} in Fortran.  When @option{-fopenmp} is specified, the
 compiler generates parallel code according to the OpenMP Application
 Program Interface v4.5 @w{@uref{https://www.openmp.org}}.  This option
@@ -2749,9 +2750,12 @@ 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 @code{simd}, @code{declare simd},
+@code{declare reduction}, @code{assume}, @code{ordered}, @code{scan},
+@code{loop} directives and combined or composite directives with
+@code{simd} as constituent with @code{#pragma omp} in C/C++,
+@code{[[omp::directive(...)]]} and @code{[[omp::sequence(...)]]} in C++
+and @code{!$omp} in Fortran.  Other OpenMP directives are ignored.
 
 @item -fpermitted-flt-eval-methods=@var{style}
 @opindex fpermitted-flt-eval-methods
diff --git a/gcc/fortran/parse.cc b/gcc/fortran/parse.cc
index 5b13441912a..2e2e9770520 100644
--- a/gcc/fortran/parse.cc
+++ b/gcc/fortran/parse.cc
@@ -924,7 +924,7 @@ decode_omp_directive (void)
       matcho ("end distribute", gfc_match_omp_eos_error, ST_OMP_END_DISTRIBUTE);
       matchs ("end do simd", gfc_match_omp_end_nowait, ST_OMP_END_DO_SIMD);
       matcho ("end do", gfc_match_omp_end_nowait, ST_OMP_END_DO);
-      matcho ("end loop", gfc_match_omp_eos_error, ST_OMP_END_LOOP);
+      matchs ("end loop", gfc_match_omp_eos_error, ST_OMP_END_LOOP);
       matchs ("end simd", gfc_match_omp_eos_error, ST_OMP_END_SIMD);
       matcho ("end masked taskloop simd", gfc_match_omp_eos_error,
 	      ST_OMP_END_MASKED_TASKLOOP_SIMD);
@@ -1023,7 +1023,7 @@ decode_omp_directive (void)
       matcho ("nothing", gfc_match_omp_nothing, ST_NONE);
       break;
     case 'l':
-      matcho ("loop", gfc_match_omp_loop, ST_OMP_LOOP);
+      matchs ("loop", gfc_match_omp_loop, ST_OMP_LOOP);
       break;
     case 'o':
       if (gfc_match ("ordered depend (") == MATCH_YES
@@ -1070,7 +1070,7 @@ decode_omp_directive (void)
       matcho ("requires", gfc_match_omp_requires, ST_OMP_REQUIRES);
       break;
     case 's':
-      matcho ("scan", gfc_match_omp_scan, ST_OMP_SCAN);
+      matchs ("scan", gfc_match_omp_scan, ST_OMP_SCAN);
       matcho ("scope", gfc_match_omp_scope, ST_OMP_SCOPE);
       matcho ("sections", gfc_match_omp_sections, ST_OMP_SECTIONS);
       matcho ("section", gfc_match_omp_eos_error, ST_OMP_SECTION);
diff --git a/gcc/testsuite/gfortran.dg/gomp/openmp-simd-7.f90 b/gcc/testsuite/gfortran.dg/gomp/openmp-simd-7.f90
new file mode 100644
index 00000000000..d7010bb4288
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/gomp/openmp-simd-7.f90
@@ -0,0 +1,23 @@
+! { dg-options "-fno-openmp -fopenmp-simd -fdump-tree-original" }
+
+subroutine foo (a, b)
+  integer, contiguous :: a(:), b(:)
+  integer :: i
+  !$omp simd reduction (inscan, +:r)
+  do i = 1, 1024
+    r = r + a(i)
+    !$omp scan inclusive(r)
+    b(i) = r
+  end do
+  !$omp end simd
+
+  !$omp loop
+  do i = 1, 1024
+    a(i) = a(i) + i
+  end do
+  !$omp end loop
+end
+
+! { dg-final { scan-tree-dump "#pragma omp simd linear\\(i:1\\) reduction\\(inscan,\\+:r\\)" "original" } }
+! { dg-final { scan-tree-dump "#pragma omp scan inclusive\\(r\\)" "original" } }
+! { dg-final { scan-tree-dump "#pragma omp loop" "original" } }