[Frontend][OpenMP] Follow compound construct clause restrictions

[skatrak]

This patch removes from the list of allowed clauses for a handful of compound constructs those that are specifically disallowed by the OpenMP spec. In particular, the following restrictions are followed:

• (regarding combined constructs) If directive-name-A is target, the copyin clause must not be specified.
• (regarding composite constructs) If directive-name-A is distribute, the ordered clause must not be specified.

These restrictions are listed in the OpenMP Specification version 5.2, sections 17.4 and 17.5. Since it's a similar case as PR #90754, I'm adding people involved in that decision as reviewers here.

[llvmbot]

@llvm/pr-subscribers-flang-fir-hlfir
@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-semantics

Author: Sergio Afonso (skatrak)

Changes

This patch removes from the list of allowed clauses for a handful of compound constructs those that are specifically disallowed by the OpenMP spec. In particular, the following restrictions are followed:

• (regarding combined constructs) If directive-name-A is target, the copyin clause must not be specified.
• (regarding composite constructs) If directive-name-A is distribute, the ordered clause must not be specified.

These restrictions are listed in the OpenMP Specification version 5.2, sections 17.4 and 17.5. Since it's a similar case as PR #90754, I'm adding people involved in that decision as reviewers here.

---

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

4 Files Affected:

• (modified) flang/test/Lower/OpenMP/distribute-parallel-do.f90 (+5-5)
• (modified) flang/test/Semantics/OpenMP/combined-constructs.f90 (+1)
• (modified) flang/test/Semantics/OpenMP/ordered03.f90 (+1)
• (modified) llvm/include/llvm/Frontend/OpenMP/OMP.td (-9)

diff --git a/flang/test/Lower/OpenMP/distribute-parallel-do.f90 b/flang/test/Lower/OpenMP/distribute-parallel-do.f90
index 91940acceb12e7..48567a1fb34913 100644
--- a/flang/test/Lower/OpenMP/distribute-parallel-do.f90
+++ b/flang/test/Lower/OpenMP/distribute-parallel-do.f90
@@ -36,21 +36,21 @@ subroutine distribute_parallel_do_dist_schedule()
   !$omp end teams
 end subroutine distribute_parallel_do_dist_schedule
 
-! CHECK-LABEL: func.func @_QPdistribute_parallel_do_ordered(
-subroutine distribute_parallel_do_ordered()
+! CHECK-LABEL: func.func @_QPdistribute_parallel_do_schedule(
+subroutine distribute_parallel_do_schedule()
   !$omp teams
 
   ! CHECK:      omp.parallel private({{.*}}) {
   ! CHECK:      omp.distribute {
-  ! CHECK-NEXT: omp.wsloop ordered(1) {
+  ! CHECK-NEXT: omp.wsloop schedule(runtime) {
   ! CHECK-NEXT: omp.loop_nest
-  !$omp distribute parallel do ordered(1)
+  !$omp distribute parallel do schedule(runtime)
   do index_ = 1, 10
   end do
   !$omp end distribute parallel do
 
   !$omp end teams
-end subroutine distribute_parallel_do_ordered
+end subroutine distribute_parallel_do_schedule
 
 ! CHECK-LABEL: func.func @_QPdistribute_parallel_do_private(
 subroutine distribute_parallel_do_private()
diff --git a/flang/test/Semantics/OpenMP/combined-constructs.f90 b/flang/test/Semantics/OpenMP/combined-constructs.f90
index 35ab6fcac58b99..b7a38482159307 100644
--- a/flang/test/Semantics/OpenMP/combined-constructs.f90
+++ b/flang/test/Semantics/OpenMP/combined-constructs.f90
@@ -100,6 +100,7 @@ program main
   enddo
   !$omp end target parallel do
 
+  !ERROR: COPYIN clause is not allowed on the TARGET PARALLEL DO directive
   !ERROR: Non-THREADPRIVATE object 'a' in COPYIN clause
   !$omp target parallel do copyin(a)
   do i = 1, N
diff --git a/flang/test/Semantics/OpenMP/ordered03.f90 b/flang/test/Semantics/OpenMP/ordered03.f90
index 8dd4d035212d8a..18f85fc24a9fb4 100644
--- a/flang/test/Semantics/OpenMP/ordered03.f90
+++ b/flang/test/Semantics/OpenMP/ordered03.f90
@@ -52,6 +52,7 @@ subroutine sub1()
   end do
   !$omp end target parallel do
 
+  !ERROR: ORDERED clause is not allowed on the TARGET TEAMS DISTRIBUTE PARALLEL DO directive
   !$omp target teams distribute parallel do ordered(1)
   do i = 1, N
     !ERROR: An ORDERED construct with the DEPEND clause must be closely nested in a worksharing-loop (or parallel worksharing-loop) construct with ORDERED clause with a parameter
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMP.td b/llvm/include/llvm/Frontend/OpenMP/OMP.td
index 2cbcbd883fb2e3..1f747df43eeb80 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMP.td
+++ b/llvm/include/llvm/Frontend/OpenMP/OMP.td
@@ -1192,7 +1192,6 @@ def OMP_DistributeParallelDo : Directive<"distribute parallel do"> {
     VersionedClause<OMPC_If>,
     VersionedClause<OMPC_NumThreads>,
     VersionedClause<OMPC_Order, 50>,
-    VersionedClause<OMPC_Ordered>,
     VersionedClause<OMPC_ProcBind>,
     VersionedClause<OMPC_Schedule>,
   ];
@@ -1293,7 +1292,6 @@ def OMP_DistributeSimd : Directive<"distribute simd"> {
     VersionedClause<OMPC_If, 50>,
     VersionedClause<OMPC_NumThreads>,
     VersionedClause<OMPC_Order, 50>,
-    VersionedClause<OMPC_Ordered>,
     VersionedClause<OMPC_ProcBind>,
     VersionedClause<OMPC_SafeLen>,
     VersionedClause<OMPC_Schedule>,
@@ -1840,7 +1838,6 @@ def OMP_TargetParallel : Directive<"target parallel"> {
 def OMP_TargetParallelDo : Directive<"target parallel do"> {
   let allowedClauses = [
     VersionedClause<OMPC_Allocator>,
-    VersionedClause<OMPC_Copyin>,
     VersionedClause<OMPC_Default>,
     VersionedClause<OMPC_Depend>,
     VersionedClause<OMPC_FirstPrivate>,
@@ -1977,7 +1974,6 @@ def OMP_TargetParallelForSimd : Directive<"target parallel for simd"> {
 def OMP_target_parallel_loop : Directive<"target parallel loop"> {
   let allowedClauses = [
     VersionedClause<OMPC_Allocate>,
-    VersionedClause<OMPC_Copyin>,
     VersionedClause<OMPC_Depend>,
     VersionedClause<OMPC_Device>,
     VersionedClause<OMPC_FirstPrivate>,
@@ -2106,7 +2102,6 @@ def OMP_TargetTeamsDistributeParallelDo :
     Directive<"target teams distribute parallel do"> {
   let allowedClauses = [
     VersionedClause<OMPC_Allocate>,
-    VersionedClause<OMPC_Copyin>,
     VersionedClause<OMPC_Depend>,
     VersionedClause<OMPC_FirstPrivate>,
     VersionedClause<OMPC_HasDeviceAddr, 51>,
@@ -2115,7 +2110,6 @@ def OMP_TargetTeamsDistributeParallelDo :
     VersionedClause<OMPC_LastPrivate>,
     VersionedClause<OMPC_Linear>,
     VersionedClause<OMPC_Map>,
-    VersionedClause<OMPC_Ordered>,
     VersionedClause<OMPC_Private>,
     VersionedClause<OMPC_Reduction>,
     VersionedClause<OMPC_Shared>,
@@ -2143,7 +2137,6 @@ def OMP_TargetTeamsDistributeParallelDoSimd :
   let allowedClauses = [
     VersionedClause<OMPC_Aligned>,
     VersionedClause<OMPC_Allocate>,
-    VersionedClause<OMPC_Copyin>,
     VersionedClause<OMPC_Depend>,
     VersionedClause<OMPC_FirstPrivate>,
     VersionedClause<OMPC_HasDeviceAddr, 51>,
@@ -2153,7 +2146,6 @@ def OMP_TargetTeamsDistributeParallelDoSimd :
     VersionedClause<OMPC_Linear>,
     VersionedClause<OMPC_Map>,
     VersionedClause<OMPC_NonTemporal>,
-    VersionedClause<OMPC_Ordered>,
     VersionedClause<OMPC_Private>,
     VersionedClause<OMPC_Reduction>,
     VersionedClause<OMPC_Shared>,
@@ -2395,7 +2387,6 @@ def OMP_TeamsDistributeParallelDo :
     VersionedClause<OMPC_NumTeams>,
     VersionedClause<OMPC_NumThreads>,
     VersionedClause<OMPC_Order, 50>,
-    VersionedClause<OMPC_Ordered>,
     VersionedClause<OMPC_ProcBind>,
     VersionedClause<OMPC_Schedule>,
     VersionedClause<OMPC_ThreadLimit>,

[mjklemm]

LGTM

[kparzysz]

In OMP.td the clauses on compound constructs should be unions of clauses from the leaf constructs. This is the default assumption in the spec. Special case restrictions, like the ones from 17.4 should be handled in separate checks.

[skatrak]

In OMP.td the clauses on compound constructs should be unions of clauses from the leaf constructs. This is the default assumption in the spec. Special case restrictions, like the ones from 17.4 should be handled in separate checks.

Did you have a particular suggestion in mind? I can think of perhaps updating the GenerateIsAllowedClause tablegen method to detect these edge cases based on the name of the directive and clause, but I don't think that's a good solution at all. Otherwise, it seems to me that your suggestion would have to be implemented as semantic checks for both clang and flang, which I don't think is better than what I proposed here.

I guess I don't quite get what the problem is with embedding these restrictions as part of the list of allowedClauses of compound constructs. I get it for dynamic restrictions based on what other clauses are present, what parent constructs there are and so on. But, for example, copyin is never accepted as a clause to a target compound construct, so it shouldn't be in the list of allowedClauses even if it would be applicable to some of its leaf constructs. In fact, before this PR, for example OMP_TargetParallelFor already did not include OMPC_Copyin (and most of the changes here are to fortran-only constructs that include these clauses clang-only variants do not).

[kparzysz]

There are already checks in both flang and clang for specific cases like this. It would be nice to have it consolidated in a single place, but the frontends handle it in their own ways (e.g. emit their own diagnostics), so doing it in a single place that accommodates the differences in the frontends may be a bit more involved.
Unless we're trying to unify handling of invalid combinations of clauses/directives, I think it's better to follow the current state.

When the OMP.td is auto-generated (in the future), it won't contain information about such restrictions. Those are not reflected in the source (JSON) files in the spec.

[skatrak]

I see, I imagined your concern could be related to the future auto-generated OMP.td. The problem is that the current approach is actually not having frontend-specific checks for this specifically, but rather omitting clauses from the allowedClauses property. As reference, see these cases prior to this PR:

• No OMPC_Copyin because of the target leaf: OMP_TargetParallel, OMP_TargetParallelDoSimd, OMP_TargetParallelFor, OMP_TargetParallelForSimd, OMP_TargetTeamsDistributeParallelFor, OMP_TargetTeamsDistributeParallelForSimd.
• No OMPC_Ordered because of the distribute leaf: OMP_DistributeParallelDoSimd, OMP_DistributeParallelFor, OMP_DistributeParallelForSimd, OMP_TargetTeamsDistribute, OMP_TargetTeamsDistributeParallelFor, OMP_TargetTeamsDistributeParallelForSimd, OMP_TargetTeamsDistributeSimd, OMP_TeamsDistributeParallelDoSimd, OMP_TeamsDistributeParallelFor, OMP_TeamsDistributeParallelForSimd, OMP_TeamsDistributeSimd.

This PR just completes these checks, since there were fewer exceptions missing than exceptions handled. If these checks need to be migrated to frontend checks because of the limited information available from these JSON files, I'm not against that. I just think that's a separate PR, what do you think?

[kparzysz]

Ok, but let not forget about this.