Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Flang][LLVM][OpenMP] Relax target data restrictions to be more inline with the specification #82537

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

agozillon
Copy link
Contributor

Currently we emit errors whenever a map is not provided on a target data directive, however, I believe that's incorrect behavior, the specification states:

"At least one map, use_device_addr or use_device_ptr clause must appear on the directive"

So provided one is present, the directive is legal in this case. Slightly different to its siblings (enter/exit/update) which don't have use_device_addr/use_device_ptr.

…ne with the specification

Currently we emit errors whenever a map is not provided on a target data
directive, however, that's incorrect behaviour,  the specification states:

"At least one map, use_device_addr or use_device_ptr clause must appear on the directive"

So provided one is present, the directive is legal in this case. Slightly different
to its siblings which don't have use_device_addr/use_device_ptr.
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:openmp flang:semantics clang:openmp OpenMP related changes to Clang labels Feb 21, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 21, 2024

@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-openmp

Author: None (agozillon)

Changes

Currently we emit errors whenever a map is not provided on a target data directive, however, I believe that's incorrect behavior, the specification states:

"At least one map, use_device_addr or use_device_ptr clause must appear on the directive"

So provided one is present, the directive is legal in this case. Slightly different to its siblings (enter/exit/update) which don't have use_device_addr/use_device_ptr.


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

2 Files Affected:

  • (modified) flang/test/Semantics/OpenMP/device-constructs.f90 (+11-1)
  • (modified) llvm/include/llvm/Frontend/OpenMP/OMP.td (+3-5)
diff --git a/flang/test/Semantics/OpenMP/device-constructs.f90 b/flang/test/Semantics/OpenMP/device-constructs.f90
index 51f00700b6daf7..1ac00ef922c6bd 100644
--- a/flang/test/Semantics/OpenMP/device-constructs.f90
+++ b/flang/test/Semantics/OpenMP/device-constructs.f90
@@ -2,9 +2,11 @@
 ! Check OpenMP clause validity for the following directives:
 !     2.10 Device constructs
 program main
+   use iso_c_binding
 
   real(8) :: arrayA(256), arrayB(256)
   integer :: N
+  type(c_ptr) :: cptr
 
   arrayA = 1.414
   arrayB = 3.14
@@ -135,7 +137,15 @@ program main
   enddo
   !$omp end target data
 
-  !ERROR: At least one of MAP clause must appear on the TARGET DATA directive
+  !$omp target data device(0) use_device_addr(cptr)
+   cptr = c_null_ptr
+  !$omp end target data
+
+  !$omp target data device(0) use_device_addr(cptr)
+   cptr = c_null_ptr
+  !$omp end target data
+
+  !ERROR: At least one of MAP, USE_DEVICE_ADDR, 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 1481328bf483b8..f44e8f18c99f6a 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMP.td
+++ b/llvm/include/llvm/Frontend/OpenMP/OMP.td
@@ -710,16 +710,14 @@ def OMP_Requires : Directive<"requires"> {
 }
 def OMP_Nothing : Directive<"nothing"> {}
 def OMP_TargetData : Directive<"target data"> {
-  let allowedClauses = [
-    VersionedClause<OMPC_UseDevicePtr>,
-    VersionedClause<OMPC_UseDeviceAddr, 50>
-  ];
   let allowedOnceClauses = [
     VersionedClause<OMPC_Device>,
     VersionedClause<OMPC_If>
   ];
   let requiredClauses = [
-    VersionedClause<OMPC_Map>
+    VersionedClause<OMPC_Map>,
+    VersionedClause<OMPC_UseDevicePtr>,
+    VersionedClause<OMPC_UseDeviceAddr, 50> 
   ];
 }
 def OMP_TargetEnterData : Directive<"target enter data"> {

@agozillon
Copy link
Contributor Author

@DominikAdamski found this and another target data related case while looking at the GenASiS benchmark, which I will try to address in a seperate PR in the near future

Copy link
Contributor

@raghavendhra raghavendhra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@skatrak skatrak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, LGTM as well!

Copy link
Contributor

@DominikAdamski DominikAdamski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@agozillon
Copy link
Contributor Author

Small follow up NFC commit to remove the extra space I added by accident (need to find some kind of space highlighting extension for vscode...)

@agozillon agozillon merged commit cf8fc53 into llvm:main Feb 22, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:openmp OpenMP related changes to Clang flang:openmp flang:semantics flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants