[LLVM][OpenMP] Add "nowait" clause as valid for "workshare"

[kparzysz]

Add the "nowait" clause to the list of allowed clauses for the "workshare"
directive. This will make it consistent with other directives (which are
shared between C/C++ and Fortran).

The parser will still reject "nowait" on "!$omp workshare", so this has no
effect on accepting/rejecting Fortran source code.

[llvmbot]

@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-openmp

Author: Krzysztof Parzyszek (kparzysz)

Changes

Add the "if" clause to the list of allowed clauses for the "workshare" directive.

---

Full diff: https://github.com/llvm/llvm-project/pull/88426.diff

1 Files Affected:

• (modified) llvm/include/llvm/Frontend/OpenMP/OMP.td (+3)

diff --git a/llvm/include/llvm/Frontend/OpenMP/OMP.td b/llvm/include/llvm/Frontend/OpenMP/OMP.td
index d9a931438b4292..e91169e8da1aa5 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMP.td
+++ b/llvm/include/llvm/Frontend/OpenMP/OMP.td
@@ -2115,6 +2115,9 @@ def OMP_scope : Directive<"scope"> {
   let association = AS_Block;
 }
 def OMP_Workshare : Directive<"workshare"> {
+  let allowedOnceClauses = [
+    VersionedClause<OMPC_NoWait>
+  ];
   let association = AS_Block;
 }
 def OMP_ParallelWorkshare : Directive<"parallel workshare"> {

[carlobertolli]

LGTM: nowait has been part of OpenMP since 4.5 (or earlier, I didn't check 4.0).
It'd be nice if you added a clang/test/OpenMP minimal test. I checked and I couldn't find a single parse test for the workshare directive.

[kparzysz]

I can add a flang test for it (clang won't likely handle it, since "workshare" is Fortran-only).

Edit: turns out there is a semantics test for it already (line 339, flang/test/Semantics/OpenMP/clause-validity01.f90):

334 ! 2.7.4 workshare
335
336   !$omp parallel
337   !$omp workshare
338   a = 1.0
339   !$omp end workshare nowait
340   !ERROR: NUM_THREADS clause is not allowed on the WORKSHARE directive
341   !$omp workshare num_threads(4)
342   a = 1.0

[kiranchandramohan]

LG.

[kiranchandramohan]

Sorry. It is already there in "End Workshare".

[kparzysz]

Sorry. It is already there in "End Workshare".

All directives that have "DIR/END DIR" format in Fortran, allow the clauses from the "END DIR" to be present in "DIR" (since there is no "END DIR" in C/C++). Workshare (being Fortran-only) is the only one that doesn't allow it.

Not having the consistency makes it harder to handle OpenMP constructs with language-agnostic code.

[kiranchandramohan]

That makes sense. Please wait for @mjklemm or @kkwli before proceeding. If you are going ahead might be good to call this out in https://github.com/llvm/llvm-project/blob/main/flang/docs/Extensions.md

Looking at other compilers. gfortran seems to accept it, ifx and classic flang rejects.

Not having the consistency makes it harder to handle OpenMP constructs with language-agnostic code.

It wasn't clear from the title that this is deviating from the standard specification for ease of implementation.

Add the "if" clause to the list of allowed clauses for the "workshare" directive.

"if" -> "nowait"

[kkwli]

As far as I understand, nowait is only allowed on the end workshare directive in the spec. However, I have no objection to extend it to have the clause on the workshare directive as long as it also allowed on the end workshare directive. However, we probably want to add semantic check for having nowait on both workshare and end workshare. How about the copyprivate clause on the single directive?

[kparzysz]

It wasn't clear from the title that this is deviating from the standard specification for ease of implementation.

It was a good catch on your side. I was testing my code that relied on this (which I had forgotten about). When I created the PR I honestly thought that this is where the "nowait" clause was supposed to be. Only after you pointed out the "end workshare" when I realized what really happened.

[kparzysz]

However, we probably want to add semantic check for having nowait on both workshare and end workshare. How about the copyprivate clause on the single directive?

The flang parser actually checks for such cases explicitly, and rejects cases of "nowait" on directives that allow it based on the .td contents, but where Fortran syntax requires them to be on the "end" directive. I added "workshare" to the list, and updated the testcase. I have checked that "copyprivate" on "!$omp single" is rejected as well.

In the current form this PR will not have any effect on the accepted syntax, it will simply allow "nowait" to be associated with "workshare" internally to the compiler.

[mjklemm]

As far as I understand, nowait is only allowed on the end workshare directive in the spec.

Post-merge comment: this was changed with OpenMP 5.2: