From patchwork Tue Oct 4 12:26:13 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tobias Burnus X-Patchwork-Id: 58765 From: tobias@codesourcery.com (Tobias Burnus) Date: Tue, 4 Oct 2022 14:26:13 +0200 Subject: [Patch] Fortran: Add OpenMP's assume(s) directives In-Reply-To: References: Message-ID: 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: --- 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},