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][OpenMP][Semantics] Modify errors to warnings for semantic checks in IS_DEVICE_PTR related to list-items being dummy arguments. #74370

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

raghavendhra
Copy link
Contributor

@raghavendhra raghavendhra commented Dec 4, 2023

This change is blocker for #71255

Changed semantic check from giving error to giving a warning about deprecation from OpenMP 5.2 and later about checks for dummy argument list-items present on IS_DEVICE_PTR clause.

@llvmbot llvmbot added flang Flang issues not falling into any other category flang:openmp flang:semantics labels Dec 4, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 4, 2023

@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-openmp

Author: Raghu Maddhipatla (raghavendhra)

Changes

Modifying #71255

Changed semantic check to only check for, "Variable in IS_DEVICE_PTR clause must be a dummy argument that does not have the ALLOCATABLE, POINTER or VALUE attribute." Earlier there was an additional semantic check "Variable in IS_DEVICE_PTR clause must be a dummy argument" which might be incorrect.


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

2 Files Affected:

  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+2-5)
  • (modified) flang/test/Semantics/OpenMP/target01.f90 (-1)
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 2f4eb9a854270..c2362d225130d 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -2994,11 +2994,8 @@ void OmpStructureChecker::Enter(const parser::OmpClause::IsDevicePtr &x) {
         context_.Say(itr->second->source,
             "Variable '%s' in IS_DEVICE_PTR clause must be of type C_PTR"_err_en_US,
             source.ToString());
-      } else if (!(IsDummy(*symbol))) {
-        context_.Say(itr->second->source,
-            "Variable '%s' in IS_DEVICE_PTR clause must be a dummy argument"_err_en_US,
-            source.ToString());
-      } else if (IsAllocatableOrPointer(*symbol) || IsValue(*symbol)) {
+      } else if (IsDummy(*symbol) &&
+          (IsAllocatableOrPointer(*symbol) || IsValue(*symbol))) {
         context_.Say(itr->second->source,
             "Variable '%s' in IS_DEVICE_PTR clause must be a dummy argument that does not have the ALLOCATABLE, POINTER or VALUE attribute."_err_en_US,
             source.ToString());
diff --git a/flang/test/Semantics/OpenMP/target01.f90 b/flang/test/Semantics/OpenMP/target01.f90
index 485fa1f2530c3..2ce9a1af7cc80 100644
--- a/flang/test/Semantics/OpenMP/target01.f90
+++ b/flang/test/Semantics/OpenMP/target01.f90
@@ -39,7 +39,6 @@ subroutine bar(b1, b2, b3)
   type(c_ptr), pointer :: b2
   type(c_ptr), value :: b3
 
-  !ERROR: Variable 'c' in IS_DEVICE_PTR clause must be a dummy argument
   !$omp target is_device_ptr(c)
     y = y + 1
   !$omp end target

@kiranchandramohan
Copy link
Contributor

Could you add a reference to the Section from the standard that asks for this restriction?

@raghavendhra
Copy link
Contributor Author

Could you add a reference to the Section from the standard that asks for this restriction?

https://www.openmp.org/spec-html/5.0/openmpsu60.html

A list item that appears in an is_device_ptr clause must be a dummy argument that does not have the ALLOCATABLE, POINTER or VALUE attribute.

@@ -39,7 +39,6 @@ subroutine bar(b1, b2, b3)
type(c_ptr), pointer :: b2
type(c_ptr), value :: b3

!ERROR: Variable 'c' in IS_DEVICE_PTR clause must be a dummy argument
Copy link
Member

Choose a reason for hiding this comment

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

According to the restriction, this error should be reported right? It says argument must be a dummy argument. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this restriction is something on the lines of "If the list-item in an is_device_ptr clause is a dummy argument it should not have ALLOCATABLE, POINTER or VALUE attributes."

I think this following text might not have completely meant that all list-items must be dummy arguments only I think the conjunction of "does not have the ALLOCATABLE, POINTER or VALUE attribute" might be the reason for the "must".
A list item that appears in an is_device_ptr clause must be a dummy argument that does not have the ALLOCATABLE, POINTER or VALUE attribute.

Also, this restriction is not present from OpenMP 5.1 onwards through which I assumed it might not be originally intended for OpenMP spec committee might not have intended to restricted list-item of is_device_ptr clause to be dummy argument only.

Please correct me if I am interpreting it incorrectly. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

So it seems like the type of list item became more restrictive as the standard progressed.

OpenMP 4.5 standard (2.10.4): A list item that appears in an is_device_ptr clause must be a dummy argument

OpenMP 5.0 standard (2.12.5): A list item that appears in an is_device_ptr clause must be a dummy argument that does not have the ALLOCATABLE, POINTER or VALUE attribute.

However, my understanding of both these statements is leaning similar to @shraiysh's- any list item in this clause has to be a dummy argument, but without any additional attributes from ALLOCATABLE, POINTER, VALUE. In other words, I don't think non-dummy arguments are allowed in this clause. This becomes clear when we look at the OpenMP 4.5 standard wording: it is, without a doubt, restricting all list items within the clause to be dummy arguments.

Copy link
Member

@shraiysh shraiysh Dec 6, 2023

Choose a reason for hiding this comment

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

Like Raghu mentioned, I cannot find this check, or anything related to the c_ptr being a dummy argument in the OpenMP 5.2 spec. If this check is not required in 5.2, we probably should remove this check completely - please let me know if there is anything related to this in 5.2. As discussed in the meeting, OpenMP 5.2 is essentially a fix on OpenMP 5.1 and OpenMP 5.0, so we should stick to OpenMP 5.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I asked @mjklemm who is CEO of OpenMP ARB

Here are his comments:

@mjklemm : 5.0 is broken. 5.1/5.2 has fixed it. OpenMP 6.0 will have a different behavior. I wonder if it would make sense to directly implement the 5.2 and 6.0 behavior.

This aligns with @shraiysh 's comment. Even I could not find anything related to dummy argument list item of IS_DEVICE_PTR clause. So, I will remove the dummy argument check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think in 5.2 we deprecate all the other use of the is_device_ptr clause in Fortran except type(c_ptr) variables in order to avoid any confusion between pointer and nonpointer list item. The has_device_address clause should be the choice for Fortran unless it is a type(c_ptr) variable.

In Section 5.4.7, we have:

If the is_device_ptr clause is specified on a target construct, if any list item is not of type C_PTR, the behavior is as if the list item appeared in a has_device_addr clause. Support for such list items in an is_device_ptr clause is deprecated.

In my opinion, issuing a warning message about the use being deprecated is good for the users. Furthermore, I don't think the spec wants to add more Fortran stuff to the is_device_ptr clause as it creates so much confusion in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed all your review comments and updated my patch. Can you please review? Thanks!

…cks in IS_DEVICE_PTR related to list-items being dummy arguments.
@raghavendhra raghavendhra changed the title [flang][OpenMP][Semantics] Modify a semantic check which was tightly checking list items in IS_DEVICE_PTR to be only dummy arguments. [Flang][OpenMP][Semantics] Modify errors to warnings for semantic checks in IS_DEVICE_PTR related to list-items being dummy arguments. Dec 7, 2023
Copy link
Contributor

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

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

LG.

@raghavendhra raghavendhra merged commit decf027 into llvm:main Dec 8, 2023
4 checks passed
@raghavendhra raghavendhra deleted the semantic_is_device_ptr branch December 8, 2023 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

6 participants