[Flang] Add semantics checks for cray pointer usage in DSA list #123171

Thirumalai-Shaktivel · 2025-01-16T09:05:38Z

Fix failure caused by this PR: #121028

Failure:
https://lab.llvm.org/buildbot/#/builders/89/builds/14474

Problems: - Cray pointee cannot be used in the DSA list - Cray pointer has to be in DSA list when cray pointee is used in the default (none) region Fix: Added required semantic checks along the tests

llvmbot · 2025-01-16T09:06:27Z

@llvm/pr-subscribers-flang-openmp

Author: Thirumalai Shaktivel (Thirumalai-Shaktivel)

Changes

Fix failure caused by this PR: #121028

Failure:
https://lab.llvm.org/buildbot/#/builders/89/builds/14474

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

1 Files Affected:

(modified) flang/lib/Semantics/resolve-directives.cpp (+4-6)

diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 9cbc61391ba1fb..85159cb5ce0685 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2115,12 +2115,10 @@ void OmpAttributeVisitor::Post(const parser::OpenMPAllocatorsConstruct &x) {
 static bool IsPrivatizable(const Symbol *sym) {
   auto *misc{sym->detailsIf<MiscDetails>()};
   return IsVariableName(*sym) && !IsProcedure(*sym) && !IsNamedConstant(*sym) &&
-      (!semantics::IsAssumedSizeArray(
-           *sym) || /* OpenMP 5.2, 5.1.1: Assumed-size arrays are shared*/
-          (sym->test(Symbol::Flag::CrayPointee) &&
-              // If CrayPointer is among the DSA list then the
-              // CrayPointee is Privatizable
-              &semantics::GetCrayPointer(*sym))) &&
+      (!semantics::IsAssumedSizeArray(*sym) /* OpenMP 5.2, 5.1.1: Assumed-size
+          arrays are shared */ ||
+          sym->test(Symbol::Flag::CrayPointee) /* If CrayPointer is among the
+          DSA list then the CrayPointee is Privatizable */) &&
       !sym->owner().IsDerivedType() &&
       sym->owner().kind() != Scope::Kind::ImpliedDos &&
       !sym->detailsIf<semantics::AssocEntityDetails>() &&

llvmbot · 2025-01-16T09:06:27Z

@llvm/pr-subscribers-flang-semantics

Author: Thirumalai Shaktivel (Thirumalai-Shaktivel)

Changes

Fix failure caused by this PR: #121028

Failure:
https://lab.llvm.org/buildbot/#/builders/89/builds/14474

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

1 Files Affected:

(modified) flang/lib/Semantics/resolve-directives.cpp (+4-6)

diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 9cbc61391ba1fb..85159cb5ce0685 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2115,12 +2115,10 @@ void OmpAttributeVisitor::Post(const parser::OpenMPAllocatorsConstruct &x) {
 static bool IsPrivatizable(const Symbol *sym) {
   auto *misc{sym->detailsIf<MiscDetails>()};
   return IsVariableName(*sym) && !IsProcedure(*sym) && !IsNamedConstant(*sym) &&
-      (!semantics::IsAssumedSizeArray(
-           *sym) || /* OpenMP 5.2, 5.1.1: Assumed-size arrays are shared*/
-          (sym->test(Symbol::Flag::CrayPointee) &&
-              // If CrayPointer is among the DSA list then the
-              // CrayPointee is Privatizable
-              &semantics::GetCrayPointer(*sym))) &&
+      (!semantics::IsAssumedSizeArray(*sym) /* OpenMP 5.2, 5.1.1: Assumed-size
+          arrays are shared */ ||
+          sym->test(Symbol::Flag::CrayPointee) /* If CrayPointer is among the
+          DSA list then the CrayPointee is Privatizable */) &&
       !sym->owner().IsDerivedType() &&
       sym->owner().kind() != Scope::Kind::ImpliedDos &&
       !sym->detailsIf<semantics::AssocEntityDetails>() &&

Thirumalai-Shaktivel · 2025-01-16T09:12:08Z

I have a doubt,

Consider an example

program test
real :: pte(*)
! pointer(pte, ptr)
end program

For which, pte would be assumed rank array unless we use pointer(pte, ptr) to create CrayPointer and mark pte as CrayPointee. Is my understanding correct?

@jeanPerier @tblah @kiranchandramohan

Thirumalai-Shaktivel · 2025-01-16T09:23:24Z

flang/lib/Semantics/resolve-directives.cpp

+          !semantics::IsAssumedSizeArray(*sym) ||
+          // If CrayPointer is among the DSA list then the
+          // CrayPointee is Privatizable
+          sym->test(Symbol::Flag::CrayPointee)) &&


Here, would checking only for CrayPointee be right?

Was the compilation failure due to the use of & in &semantics::GetCrayPointer(*sym))?

Yes. It fails in flang-aarch64-libcxx and It says the following:

[...] FAILED: tools/flang/lib/Semantics/CMakeFiles/FortranSemantics.dir/resolve-directives.cpp.o /usr/local/bin/c++ -DFLANG_INCLUDE_TESTS=1 -DFLANG_LITTLE_ENDIAN=1 -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/tcwg-buildbot/worker/flang-aarch64-libcxx/build/tools/flang/lib/Semantics -I/home/tcwg-buildbot/worker/flang-aarch64-libcxx/llvm-project/flang/lib/Semantics -I/home/tcwg-buildbot/worker/flang-aarch64-libcxx/llvm-project/flang/include -I/home/tcwg-buildbot/worker/flang-aarch64-libcxx/build/tools/flang/include -I/home/tcwg-buildbot/worker/flang-aarch64-libcxx/build/include -I/home/tcwg-buildbot/worker/flang-aarch64-libcxx/llvm-project/llvm/include -isystem /home/tcwg-buildbot/worker/flang-aarch64-libcxx/llvm-project/llvm/../mlir/include -isystem /home/tcwg-buildbot/worker/flang-aarch64-libcxx/build/tools/mlir/include -isystem /home/tcwg-buildbot/worker/flang-aarch64-libcxx/build/tools/clang/include -isystem /home/tcwg-buildbot/worker/flang-aarch64-libcxx/llvm-project/llvm/../clang/include -stdlib=libc++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Werror -Wno-deprecated-copy -Wno-string-conversion -Wno-ctad-maybe-unsupported -Wno-unused-command-line-argument -Wstring-conversion -Wcovered-switch-default -Wno-nested-anon-types -O3 -DNDEBUG -std=c++17 -fPIC -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -MD -MT tools/flang/lib/Semantics/CMakeFiles/FortranSemantics.dir/resolve-directives.cpp.o -MF tools/flang/lib/Semantics/CMakeFiles/FortranSemantics.dir/resolve-directives.cpp.o.d -o tools/flang/lib/Semantics/CMakeFiles/FortranSemantics.dir/resolve-directives.cpp.o -c /home/tcwg-buildbot/worker/flang-aarch64-libcxx/llvm-project/flang/lib/Semantics/resolve-directives.cpp ../llvm-project/flang/lib/Semantics/resolve-directives.cpp:2123:16: error: reference cannot be bound to dereferenced null pointer in well-defined C++ code; pointer may be assumed to always convert to true [-Werror,-Wundefined-bool-conversion] 2120 | (sym->test(Symbol::Flag::CrayPointee) && | ~~ 2121 | // If CrayPointer is among the DSA list then the 2122 | // CrayPointee is Privatizable 2123 | &semantics::GetCrayPointer(*sym))) && | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../llvm-project/flang/include/flang/Semantics/tools.h:315:15: note: 'GetCrayPointer' returns a reference 315 | const Symbol &GetCrayPointer(const Symbol &crayPointee); | ^ 1 error generated.

kiranchandramohan · 2025-01-16T16:44:44Z

Can you revert the patch so that the bots pass?

kiranchandramohan · 2025-01-16T16:49:13Z

I have a doubt,

Consider an example
program test
real :: pte(*)
! pointer(pte, ptr)
end program
For which, pte would be assumed rank array unless we use pointer(pte, ptr) to create CrayPointer and mark pte as CrayPointee. Is my understanding correct?

@jeanPerier @tblah @kiranchandramohan

I am not very familiar with Cray pointers. Would @DanielCChen or @kkwli know?

kiranchandramohan · 2025-01-16T16:54:46Z

I have reverted the original patch (#123220) since we have not concluded. Hope it is OK.

DanielCChen · 2025-01-16T17:53:39Z

I have a doubt,
Consider an example
program test
real :: pte(*)
! pointer(pte, ptr)
end program
For which, pte would be assumed rank array unless we use pointer(pte, ptr) to create CrayPointer and mark pte as CrayPointee. Is my understanding correct?
@jeanPerier @tblah @kiranchandramohan
I am not very familiar with Cray pointers. Would @DanielCChen or @kkwli know?

The example code is not standard conforming.
If I have to guess, real :: pte(*) is trying to declare a rank-1 assumed-size dummy argument, pte.
A CrayPointer is just a C pointer. It only represents address of its pointee.

I modified the code to make it executable and hopefully it will illustrate how CrayPointer works.

real :: arr(10)
arr = 10
call sub(arr)
contains
subroutine sub(pte)
  real :: pte(*)
  real :: local(10)
  pointer (ptr, local)
  ptr = loc(pte)
  print*, local
end
end

Thirumalai-Shaktivel · 2025-01-17T03:26:09Z

Here is another example, that I was trying to compile and make it work
Godbolt: https://godbolt.org/z/MbYTx67KT

subroutine test_cray_pointer_usage
  implicit none
  real(8) :: var(*), pointee(2)
  pointer(ivar, var)

  pointee = 42.0
  ivar = loc(pointee)

  !$omp parallel num_threads(2) default(none)
    ! ERROR: The DEFAULT(NONE) clause requires that the Cray Pointer 'ivar' must be listed in a data-sharing attribute clause
    print *, var(1)
  !$omp end parallel

  ! ERROR: Cray Pointee 'var' may not appear in PRIVATE clause, use Cray Pointer 'ivar' instead
  !$omp parallel num_threads(2) default(none) private(var)
    print *, var(1)
  !$omp end parallel

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

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

Thirumalai-Shaktivel · 2025-01-17T03:27:10Z

I have reverted the original patch (#123220) since we have not concluded. Hope it is OK.

Thank you, I will push changes here.

Thirumalai-Shaktivel · 2025-01-21T08:12:37Z

Ready for review.

kkwli · 2025-01-21T19:17:53Z

Do we need to have the same check for other DSA clauses (e.g. linear)?

Thirumalai-Shaktivel · 2025-01-22T04:24:11Z

Yes. I missed them, thanks for noticing.

Apart from Linear and LastPrivate, do you see anything else missing?
Reference: https://www.openmp.org/spec-html/5.0/openmpsu106.html#:~:text=DescriptionThe%20default(shared)%20clause,sharing%20attributes%20to%20be%20firstprivate.

tblah

Code changes LGTM but I don't know enough about cray pointers to approve

kkwli · 2025-01-24T16:37:30Z

Apart from Linear and LastPrivate, do you see anything else missing?

I think linear and lastprivate are the ones missing for 5.0 support. The list of data-sharing attribute clause is extended to other clauses in 6.0 that we can leave it to another PR.

Thirumalai-Shaktivel · 2025-01-27T05:43:14Z

@kkwli This PR is ready for review!

kkwli

LGTM Thanks.

Thirumalai-Shaktivel · 2025-01-28T02:53:57Z

Thanks for the reviews!

[Flang OpenMP] Add semantics checks for cray pointer usage in DSA list

9425e5d

Problems: - Cray pointee cannot be used in the DSA list - Cray pointer has to be in DSA list when cray pointee is used in the default (none) region Fix: Added required semantic checks along the tests

llvmbot added flang Flang issues not falling into any other category flang:openmp flang:semantics labels Jan 16, 2025

Thirumalai-Shaktivel mentioned this pull request Jan 16, 2025

[Flang OpenMP] Add semantics checks for cray pointer usage in DSA list #121028

Merged

[Flang] Remove redundant check for CrayPointer

538a41c

Thirumalai-Shaktivel force-pushed the llvm/cray_ptr_02 branch from c705905 to 134129c Compare January 16, 2025 09:22

Thirumalai-Shaktivel commented Jan 16, 2025

View reviewed changes

Thirumalai-Shaktivel force-pushed the llvm/cray_ptr_02 branch from 134129c to 538a41c Compare January 17, 2025 11:14

Thirumalai-Shaktivel changed the title ~~[Flang] Remove redundant check for CrayPointer~~ [Flang] Add semantics checks for cray pointer usage in DSA list Jan 17, 2025

Thirumalai-Shaktivel requested review from tblah, jeanPerier and kiranchandramohan January 21, 2025 08:12

DanielCChen requested a review from kkwli January 21, 2025 17:02

ceseo mentioned this pull request Jan 23, 2025

[Flang][OpenMP] Compilation error when specifying a pointee (second argument) specified in cray pointer to loc intrinsic function #80632

Closed

tblah reviewed Jan 24, 2025

View reviewed changes

[Flang] Throw errors for other DSA clauses based on OpenMP 5.0

1142fdc

Merge branch 'main' into llvm/cray_ptr_02

e9e799a

kkwli approved these changes Jan 27, 2025

View reviewed changes

Thirumalai-Shaktivel merged commit ba789c6 into llvm:main Jan 28, 2025
8 checks passed

Thirumalai-Shaktivel deleted the llvm/cray_ptr_02 branch January 28, 2025 02:57

[Flang] Add semantics checks for cray pointer usage in DSA list #123171

[Flang] Add semantics checks for cray pointer usage in DSA list #123171

Uh oh!

Conversation

Thirumalai-Shaktivel commented Jan 16, 2025

Uh oh!

llvmbot commented Jan 16, 2025

Uh oh!

llvmbot commented Jan 16, 2025

Uh oh!

Thirumalai-Shaktivel commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Thirumalai-Shaktivel Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

kiranchandramohan Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

kiranchandramohan commented Jan 16, 2025

Uh oh!

kiranchandramohan commented Jan 16, 2025

Uh oh!

kiranchandramohan commented Jan 16, 2025

Uh oh!

DanielCChen commented Jan 16, 2025

Uh oh!

Thirumalai-Shaktivel commented Jan 17, 2025

Uh oh!

Thirumalai-Shaktivel commented Jan 17, 2025

Uh oh!

Thirumalai-Shaktivel commented Jan 21, 2025

Uh oh!

kkwli commented Jan 21, 2025

Uh oh!

Thirumalai-Shaktivel commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tblah left a comment

Choose a reason for hiding this comment

Uh oh!

kkwli commented Jan 24, 2025

Uh oh!

Thirumalai-Shaktivel commented Jan 27, 2025

Uh oh!

kkwli left a comment

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel commented Jan 28, 2025

Uh oh!

Uh oh!

Uh oh!

Thirumalai-Shaktivel commented Jan 16, 2025 •

edited

Loading

Thirumalai-Shaktivel commented Jan 22, 2025 •

edited

Loading