[Flang] [Semantics] [OpenMP] Added missing semantic check with nested target region.

[raghavendhra]

Issue semantic warning for any combination of nested OMP TARGET directives inside another OMP TARGET region.

This change would not affect OMP TARGET inside an OMP TARGET DATA. However, it issues warning for OMP TARGET DATA inside an OMP TARGET region.

[llvmbot]

@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-openmp

Author: Raghu Maddhipatla (raghavendhra)

Changes

Issue semantic warning for any combination of nested OMP TARGET directives inside an OMP TARGET DATA directive and vice versa.

---

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

3 Files Affected:

• (modified) flang/lib/Semantics/check-omp-structure.cpp (+16-1)
• (modified) flang/test/Semantics/OpenMP/nested-simd.f90 (+1)
• (modified) flang/test/Semantics/OpenMP/nested-target.f90 (+14-1)

diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 014604627f2cd1..d4fa32d5f26520 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -785,7 +785,8 @@ void OmpStructureChecker::CheckTargetNest(const parser::OpenMPConstruct &c) {
                 std::get<parser::OmpBeginBlockDirective>(c.t)};
             const auto &beginDir{
                 std::get<parser::OmpBlockDirective>(beginBlockDir.t)};
-            if (beginDir.v == llvm::omp::Directive::OMPD_target_data) {
+            if (beginDir.v == llvm::omp::Directive::OMPD_target_data ||
+                llvm::omp::allTargetSet.test(beginDir.v)) {
               eligibleTarget = false;
               ineligibleTargetDir = beginDir.v;
             }
@@ -984,6 +985,20 @@ void OmpStructureChecker::Enter(const parser::OpenMPBlockConstruct &x) {
     if (llvm::omp::topTeamsSet.test(GetContextParent().directive)) {
       HasInvalidTeamsNesting(beginDir.v, beginDir.source);
     }
+    if ((llvm::omp::allTargetSet.test(GetContext().directive) ||
+            (GetContext().directive ==
+                llvm::omp::Directive::OMPD_target_data)) &&
+        (llvm::omp::allTargetSet.test(GetContextParent().directive) ||
+            (GetContextParent().directive ==
+                llvm::omp::Directive::OMPD_target_data))) {
+      context_.Warn(common::UsageWarning::OpenMPUsage,
+          parser::FindSourceLocation(x),
+          "If %s directive is nested inside %s region, the behaviour is unspecified"_port_en_US,
+          parser::ToUpperCaseLetters(
+              getDirectiveName(GetContext().directive).str()),
+          parser::ToUpperCaseLetters(
+              getDirectiveName(GetContextParent().directive).str()));
+    }
     if (GetContext().directive == llvm::omp::Directive::OMPD_master) {
       CheckMasterNesting(x);
     }
diff --git a/flang/test/Semantics/OpenMP/nested-simd.f90 b/flang/test/Semantics/OpenMP/nested-simd.f90
index 4149b6d97e9dc7..73e72cd5bf6a5a 100644
--- a/flang/test/Semantics/OpenMP/nested-simd.f90
+++ b/flang/test/Semantics/OpenMP/nested-simd.f90
@@ -166,6 +166,7 @@ SUBROUTINE NESTED_BAD(N)
       end do
       !$omp end task
       !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+      !ERROR: If TARGET directive is nested inside TARGET SIMD region, the behaviour is unspecified
       !$omp target 
       do J = 1, N
         K = 2
diff --git a/flang/test/Semantics/OpenMP/nested-target.f90 b/flang/test/Semantics/OpenMP/nested-target.f90
index 2267f70715d3ed..b054292ef54d58 100644
--- a/flang/test/Semantics/OpenMP/nested-target.f90
+++ b/flang/test/Semantics/OpenMP/nested-target.f90
@@ -5,7 +5,7 @@
 ! 2.12.5 Target Construct
 
 program main
-  integer :: i, j, N = 10
+  integer :: i, j, N = 10, n1, n2, res(100)
   real :: a, arrayA(512), arrayB(512), ai(10)
   real, allocatable :: B(:)
 
@@ -50,4 +50,17 @@ program main
   !$omp end target
   deallocate(B)
 
+  n1 = 10
+  n2 = 10
+  !$omp target teams map(to:a)
+  !PORTABILITY: If TARGET DATA directive is nested inside TARGET TEAMS region, the behaviour is unspecified
+  !$omp target data map(n1,n2)
+  do i=1, n1
+     do j=1, n2
+      res((i-1)*10+j) = i*j
+     end do
+  end do
+  !$omp end target data
+  !$omp end target teams
+
 end program main

[mjklemm]

Issue semantic warning for any combination of nested OMP TARGET directives inside an OMP TARGET DATA directive and vice versa.

Is there an error in the description? It's fine to nest target in target data.

[raghavendhra]

Issue semantic warning for any combination of nested OMP TARGET directives inside an OMP TARGET DATA directive and vice versa.

Is there an error in the description? It's fine to nest target in target data.

Thank you for your review Michael!

There is already existent warning being issued for OMP TARGET DATA inside an OMP TARGET region here in the upstream
https://github.com/llvm/llvm-project/blob/a8a1e9033a902d961ad050a139b97ac0319b9e25/flang/test/Semantics/OpenMP/nested-target.f90#L32C3-L39C19

Is this existent test case is also incorrectly issuing warning and should be changed to allow OMP TARGET DATA nested inside OMP TARGET region?

[raghavendhra]

Issue semantic warning for any combination of nested OMP TARGET directives inside an OMP TARGET DATA directive and vice versa.

Is there an error in the description? It's fine to nest target in target data.

Thank you for your review Michael!

There is already existent warning being issued for OMP TARGET DATA inside an OMP TARGET region here in the upstream https://github.com/llvm/llvm-project/blob/a8a1e9033a902d961ad050a139b97ac0319b9e25/flang/test/Semantics/OpenMP/nested-target.f90#L32C3-L39C19

Is this existent test case is also incorrectly issuing warning and should be changed to allow OMP TARGET DATA nested inside OMP TARGET region?

Upon discussion with Michael Klemm offline, we decided,

PR only creates the warning for the case of target data being inside target,

[mjklemm]

Like indicated in the comment for the test, the code could be turned in valid code if the inner target construct would have device(ancestor) and was compiled with requires reverse_offload.

Do you want to handle this as part of this PR? I doubt that reverse offloading is supported at this point, so I'd be fine with the PR landing as is and deferring the extension of the semantics to a later PR, once reverse offloading will be supported.

[mjklemm]

LGTM