Skip to content

[Flang][OpenMP] : Compilation error involving Cray pointers and data sharing attributes #82481

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kiranktp
Copy link
Contributor

@kiranktp kiranktp commented Feb 21, 2024

Issue description:
        When DEFAULT DSA is NONE, all data refs must be listed in one of the data sharing clause.
        When a Cray pointer is listed in one of the data sharing clause, Cray pointee can be used in parallel region.
        This is valid as per standard.
        "Cray pointees have the same data-sharing attribute as the storage with which their Cray pointers are associated."
        Currently compiler crashes for default(none).
        Also current semantic checks incorrectly updates the symbol flags related to Craypointee symbols.
        due to this incorrect updation, compiler crashes when default(private) and default(firstprivate) is specified.

Solution:
        Added an additional check to skip updation of symbol flags related to Craypointee symbols.
        This also prevents from checking cray pointee when DEFAULT DSA is NONE.

This patch has code changes and a test case.

@kiranktp kiranktp added flang Flang issues not falling into any other category flang:openmp flang:semantics labels Feb 21, 2024
@llvmbot
Copy link
Member

llvmbot commented Feb 21, 2024

@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-openmp

Author: Kiran Kumar T P (kiranktp)

Changes

Issue description:
When DEFAULT DSA is NONE, all data refs must be listed in one of the data sharing clause.
But when a Cray pointer is listed in one of the data sharing clause, Cray pointee can be used in parallel region.
This is valid as per standard.
"Cray pointee's have the same data-sharing attribute as the storage with which their Cray pointers are associated."

Solution:
Added a check to discard checking Cray pointee when DEFAULT DSA is NONE.

This patch has code changes and a test case.


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

2 Files Affected:

  • (modified) flang/lib/Semantics/resolve-directives.cpp (+3-1)
  • (added) flang/test/Semantics/OpenMP/cray-pointer-usage.f90 (+17)
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 4b6d083671bc92..fb336853ae0c2d 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -1995,7 +1995,9 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
             // Exclude indices of sequential loops that are privatised in
             // the scope of the parallel region, and not in this scope.
             // TODO: check whether this should be caught in IsObjectWithDSA
-            !symbol->test(Symbol::Flag::OmpPrivate)) {
+            !symbol->test(Symbol::Flag::OmpPrivate) &&
+            // If the symbol is a CrayPointee, It cannot appear in DSA clause
+            !symbol->test(Symbol::Flag::CrayPointee)) {
           context_.Say(name.source,
               "The DEFAULT(NONE) clause requires that '%s' must be listed in "
               "a data-sharing attribute clause"_err_en_US,
diff --git a/flang/test/Semantics/OpenMP/cray-pointer-usage.f90 b/flang/test/Semantics/OpenMP/cray-pointer-usage.f90
new file mode 100644
index 00000000000000..5d3c7f085cc4d3
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/cray-pointer-usage.f90
@@ -0,0 +1,17 @@
+!RUN: %python %S/../test_errors.py %s %flang -fopenmp
+subroutine test_crayptr
+  implicit none
+  integer :: I
+  real*8 var(*)
+  pointer(ivar,var)
+  real*8 pointee(8)
+
+  pointee(1) = 42.0
+  ivar = loc(pointee)
+
+  !$omp parallel num_threads(2) default(none) shared(ivar)
+    print *, var(1)
+  !$omp end parallel
+
+end subroutine test_crayptr
+

Copy link
Contributor

@mjklemm mjklemm left a comment

Choose a reason for hiding this comment

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

I think the PR is not yet sufficient, as it resolves the issue of Flang issuing a fatal diagnostic about the pointee, but it does not seem to properly inherit the data-sharing attribute of the pointer.

In fact, the compiler segfaults for a case like this:

program test_crayptr
  implicit none
  real*8 var(*)
  pointer(ivar,var)
  real*8 pointee(8)

  pointee(1) = 42.0
  ivar = loc(pointee)

  !$omp parallel num_threads(2) default(none) shared(ivar)
    print *, var(1)
  !$omp end parallel

  !$omp parallel num_threads(2) default(none) private(ivar)
    print *, var(1)
  !$omp end parallel

  !$omp parallel num_threads(2) default(private) shared(ivar)
    print *, var(1)
  !$omp end parallel

  !$omp parallel num_threads(2) default(firstprivate) shared(ivar)
    print *, var(1)
  !$omp end parallel
end program test_crayptr

@kiranktp
Copy link
Contributor Author

Thanks for the Review Michael. I have encountered 3 more issues related to cray pointer usage in DSA.
I will create an upstream ticket and take care of all these scenarios at once. As there was a comment to refactor the checks for DSA. I will try to work on that as well.
This issue was a blocker for our testing. so just fixed one scenario and raised a PR. I will take care of data sharing inheritance as part of this patch.

Copy link

✅ With the latest revision this PR passed the Python code formatter.

Copy link

github-actions bot commented Mar 25, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

dpalermo added a commit to ROCm/aomp that referenced this pull request May 8, 2024
  - Currently requires manually applying:
      llvm/llvm-project#82481
  - Will move back after patch lands to avoid false alarms for developers
    who don't apply this patch
dpalermo added a commit to ROCm/aomp that referenced this pull request May 8, 2024
- Currently requires manually applying:
      llvm/llvm-project#82481
  - Will move back after patch lands to avoid false alarms for developers
    who don't apply this patch
…sharing attributes

Issue description:
	When DEFAULT DSA is NONE, all data refs must be listed in one of the data sharing clause.
	When a Cray pointer is listed in one of the data sharing clause, Cray pointee can be used in parallel region.
	This is valid as per standard.
	"Cray pointees have the same data-sharing attribute as the storage with which their Cray pointers are associated."
        Currently compiler crashes for default(none).
        Also current semantic checks incorrectly updates the symbol flags related to Craypointee symbols.
        due to this incorrect updation, compiler crashes when default(private) and default(firstprivate) is specified.

Solution:
        Added an additional check to skip updation of symbol flags related to Craypointee symbols.
        This also prevents from checking cray pointee when DEFAULT DSA is NONE.

This patch has code changes and a test case.
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.

3 participants