[flang][OpenMP] Add semantic check for target data

[shraiysh]

This patch adds the following semantic check on target data

At least one map, use_device_addr or use_device_ptr clause must appear
on the target data directive.

[llvmbot]

@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-semantics

Author: Shraiysh (shraiysh)

Changes

This patch adds the following semantic check on target data

At least one map, use_device_addr or use_device_ptr clause must appear
on the target data directive.

---

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

4 Files Affected:

• (modified) flang/lib/Semantics/check-omp-structure.cpp (+16)
• (modified) flang/lib/Semantics/check-omp-structure.h (+1)
• (modified) flang/test/Semantics/OpenMP/device-constructs.f90 (+1-1)
• (modified) llvm/include/llvm/Frontend/OpenMP/OMP.td (+2-4)

diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index d7a0681d1c3d476..1b6b38d0e972d0d 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -919,12 +919,28 @@ void OmpStructureChecker::ChecksOnOrderedAsBlock() {
   }
 }
 
+void OmpStructureChecker::CheckTargetData() {
+  const parser::OmpClause *mapClause = FindClause(llvm::omp::Clause::OMPC_map);
+  const parser::OmpClause *useDevicePtrClause =
+      FindClause(llvm::omp::Clause::OMPC_use_device_ptr);
+  const parser::OmpClause *useDeviceAddrClause =
+      FindClause(llvm::omp::OMPC_use_device_addr);
+  if (!mapClause && !useDevicePtrClause && !useDeviceAddrClause) {
+    context_.Say(GetContext().directiveSource,
+        "At least one MAP, USE_DEVICE_ADDR or USE_DEVICE_PTR clause must "
+        "appear on the TARGET DATA directive."_err_en_US);
+  }
+}
+
 void OmpStructureChecker::Leave(const parser::OmpBeginBlockDirective &) {
   switch (GetContext().directive) {
   case llvm::omp::Directive::OMPD_ordered:
     // [5.1] 2.19.9 Ordered Construct Restriction
     ChecksOnOrderedAsBlock();
     break;
+  case llvm::omp::Directive::OMPD_target_data:
+    CheckTargetData();
+    return;
   default:
     break;
   }
diff --git a/flang/lib/Semantics/check-omp-structure.h b/flang/lib/Semantics/check-omp-structure.h
index d35602cca75d549..5c80c1ea8a2d23e 100644
--- a/flang/lib/Semantics/check-omp-structure.h
+++ b/flang/lib/Semantics/check-omp-structure.h
@@ -184,6 +184,7 @@ class OmpStructureChecker
   void CheckSIMDNest(const parser::OpenMPConstruct &x);
   void CheckTargetNest(const parser::OpenMPConstruct &x);
   void CheckTargetUpdate();
+  void CheckTargetData();
   void CheckCancellationNest(
       const parser::CharBlock &source, const parser::OmpCancelType::Type &type);
   std::int64_t GetOrdCollapseLevel(const parser::OpenMPLoopConstruct &x);
diff --git a/flang/test/Semantics/OpenMP/device-constructs.f90 b/flang/test/Semantics/OpenMP/device-constructs.f90
index 51f00700b6daf74..a18d99d7c368b64 100644
--- a/flang/test/Semantics/OpenMP/device-constructs.f90
+++ b/flang/test/Semantics/OpenMP/device-constructs.f90
@@ -135,7 +135,7 @@ program main
   enddo
   !$omp end target data
 
-  !ERROR: At least one of MAP clause must appear on the TARGET DATA directive
+  !ERROR: At least one MAP, USE_DEVICE_ADDR or USE_DEVICE_PTR clause must appear on the TARGET DATA directive.
   !$omp target data device(0)
   do i = 1, N
      a = 3.14
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMP.td b/llvm/include/llvm/Frontend/OpenMP/OMP.td
index f8b3b0c7524979b..79211976c0270c1 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMP.td
+++ b/llvm/include/llvm/Frontend/OpenMP/OMP.td
@@ -705,15 +705,13 @@ def OMP_Nothing : Directive<"nothing"> {}
 def OMP_TargetData : Directive<"target data"> {
   let allowedClauses = [
     VersionedClause<OMPC_UseDevicePtr>,
-    VersionedClause<OMPC_UseDeviceAddr, 50>
+    VersionedClause<OMPC_UseDeviceAddr, 50>,
+    VersionedClause<OMPC_Map>
   ];
   let allowedOnceClauses = [
     VersionedClause<OMPC_Device>,
     VersionedClause<OMPC_If>
   ];
-  let requiredClauses = [
-    VersionedClause<OMPC_Map>
-  ];
 }
 def OMP_TargetEnterData : Directive<"target enter data"> {
   let allowedClauses = [

[kiranchandramohan]

LG.

[shraiysh]

As it turns out, this check has been removed in OpenMP 5.2 and OpenMP 6.0.
Upto OpenMP 4.0 - No check.
OpenMP 4.5 - At least one MAP clause must be present.
OpenMP 5.0, 5.1 - At lease one of MAP, USE_DEVICE_PTR or USE_DEVICE_ADDR must be present.
OpenMP 5.2, 6.0 - No Check.
This makes me feel like we should not add this check like this, but we should have a way to pass the OpenMP version information to the semantics checker. I can start working on this @kiranchandramohan , but where should this information reside? In the SemanticsContext?

[kiranchandramohan]

I think version checking will add additional complexity and will be difficult to maintain. For now, we can stick with the latest standard. Although in OpenMP this is difficult due to the standard moving fast.

[shraiysh]

Wouldn't that end up with unexpected crashes in the later stages of the compiler - where the code generation might be happening specific to the openmp version? For example, the MLIR module has an attribute with openmp version which I am assuming is used at some places - and the code generation there will assume that the restrictions are applied and checked by the frontend. clang has semantic checks based on OpenMP version.

[kiranchandramohan]

But which standards will we try to keep up with? All standards starting from 1.1? Or the recent ones?

If you would like to proceed with checking based on the standard version then please write an RFC in Flang discourse.

[shraiysh]

Hmm, that's a valid concern. We might not be able to support all the standards. Right now, it seems like a mix of OpenMP 5.0, 5.1 and 5.2.

It sounds like a long project to support checks based on OpenMP versions, and I might not be able to commit to that right now as my internship is ending soon.

Just for future reference for adding semantics checks, I should refer to OpenMP 5.2 for adding a check, right? I will close this PR as this restriction is not mentioned in OpenMP 5.2.

[NimishMishra]

But which standards will we try to keep up with? All standards starting from 1.1? Or the recent ones?

If you would like to proceed with checking based on the standard version then please write an RFC in Flang discourse.

Are we focussing mainly on OpenMP 5.0 for the first production-level release or beyond 5.0 too? Pinging @kiranktp too