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
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" } }
