[Flang OpenMP] Add semantics checks for cray pointer usage in DSA list #121028
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
|
@llvm/pr-subscribers-flang-openmp Author: Thirumalai Shaktivel (Thirumalai-Shaktivel) ChangesProblems:
Fix: Added required semantic checks along the tests Reference from the documentation (OpenMP 5.0: 2.19.1):
Full diff: https://github.com/llvm/llvm-project/pull/121028.diff 4 Files Affected:
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 95b962f5daf57c..81febfacb37483 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -3340,6 +3340,7 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Ordered &x) {
void OmpStructureChecker::Enter(const parser::OmpClause::Shared &x) {
CheckAllowedClause(llvm::omp::Clause::OMPC_shared);
CheckIsVarPartOfAnotherVar(GetContext().clauseSource, x.v, "SHARED");
+ CheckCrayPointee(x.v, "SHARED");
}
void OmpStructureChecker::Enter(const parser::OmpClause::Private &x) {
SymbolSourceMap symbols;
@@ -3347,6 +3348,7 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Private &x) {
CheckAllowedClause(llvm::omp::Clause::OMPC_private);
CheckIsVarPartOfAnotherVar(GetContext().clauseSource, x.v, "PRIVATE");
CheckIntentInPointer(symbols, llvm::omp::Clause::OMPC_private);
+ CheckCrayPointee(x.v, "PRIVATE");
}
void OmpStructureChecker::Enter(const parser::OmpClause::Nowait &x) {
@@ -3426,6 +3428,7 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Firstprivate &x) {
CheckAllowedClause(llvm::omp::Clause::OMPC_firstprivate);
CheckIsVarPartOfAnotherVar(GetContext().clauseSource, x.v, "FIRSTPRIVATE");
+ CheckCrayPointee(x.v, "FIRSTPRIVATE");
CheckIsLoopIvPartOfClause(llvmOmpClause::OMPC_firstprivate, x.v);
SymbolSourceMap currSymbols;
@@ -4522,6 +4525,22 @@ void OmpStructureChecker::CheckProcedurePointer(
}
}
+void OmpStructureChecker::CheckCrayPointee(
+ const parser::OmpObjectList &objectList, llvm::StringRef clause) {
+ SymbolSourceMap symbols;
+ GetSymbolsInObjectList(objectList, symbols);
+ for (auto it{symbols.begin()}; it != symbols.end(); ++it) {
+ const auto *symbol{it->first};
+ const auto source{it->second};
+ if (symbol->test(Symbol::Flag::CrayPointee)) {
+ context_.Say(source,
+ "Cray Pointee '%s' may not appear in %s clause, use Cray Pointer '%s' instead"_err_en_US,
+ symbol->name(), clause.str(),
+ semantics::GetCrayPointer(*symbol).name());
+ }
+ }
+}
+
void OmpStructureChecker::GetSymbolsInObjectList(
const parser::OmpObjectList &objectList, SymbolSourceMap &symbols) {
for (const auto &ompObject : objectList.v) {
diff --git a/flang/lib/Semantics/check-omp-structure.h b/flang/lib/Semantics/check-omp-structure.h
index 346a7bed9138f0..8bc37e98b3ce3b 100644
--- a/flang/lib/Semantics/check-omp-structure.h
+++ b/flang/lib/Semantics/check-omp-structure.h
@@ -194,6 +194,8 @@ class OmpStructureChecker
const parser::CharBlock &source, const parser::OmpObjectList &objList);
void CheckIntentInPointer(SymbolSourceMap &, const llvm::omp::Clause);
void CheckProcedurePointer(SymbolSourceMap &, const llvm::omp::Clause);
+ void CheckCrayPointee(
+ const parser::OmpObjectList &objectList, llvm::StringRef clause);
void GetSymbolsInObjectList(const parser::OmpObjectList &, SymbolSourceMap &);
void CheckDefinableObjects(SymbolSourceMap &, const llvm::omp::Clause);
void CheckCopyingPolymorphicAllocatable(
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 39478b58a9070d..9cbc61391ba1fb 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2115,8 +2115,12 @@ 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*/
+ (!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))) &&
!sym->owner().IsDerivedType() &&
sym->owner().kind() != Scope::Kind::ImpliedDos &&
!sym->detailsIf<semantics::AssocEntityDetails>() &&
@@ -2282,10 +2286,18 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
// 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)) {
- context_.Say(name.source,
- "The DEFAULT(NONE) clause requires that '%s' must be listed in "
- "a data-sharing attribute clause"_err_en_US,
- symbol->name());
+ if (symbol->test(Symbol::Flag::CrayPointee)) {
+ std::string crayPtrName{
+ semantics::GetCrayPointer(*symbol).name().ToString()};
+ if (!IsObjectWithDSA(*currScope().FindSymbol(crayPtrName)))
+ context_.Say(name.source,
+ "The DEFAULT(NONE) clause requires that the Cray Pointer '%s' must be listed in a data-sharing attribute clause"_err_en_US,
+ crayPtrName);
+ } else {
+ context_.Say(name.source,
+ "The DEFAULT(NONE) clause requires that '%s' must be listed in a data-sharing attribute clause"_err_en_US,
+ symbol->name());
+ }
}
}
}
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..c7d03f0db99040
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/cray-pointer-usage.f90
@@ -0,0 +1,27 @@
+!RUN: %python %S/../test_errors.py %s %flang -fopenmp
+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
|
|
@llvm/pr-subscribers-flang-semantics Author: Thirumalai Shaktivel (Thirumalai-Shaktivel) ChangesProblems:
Fix: Added required semantic checks along the tests Reference from the documentation (OpenMP 5.0: 2.19.1):
Full diff: https://github.com/llvm/llvm-project/pull/121028.diff 4 Files Affected:
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 95b962f5daf57c..81febfacb37483 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -3340,6 +3340,7 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Ordered &x) {
void OmpStructureChecker::Enter(const parser::OmpClause::Shared &x) {
CheckAllowedClause(llvm::omp::Clause::OMPC_shared);
CheckIsVarPartOfAnotherVar(GetContext().clauseSource, x.v, "SHARED");
+ CheckCrayPointee(x.v, "SHARED");
}
void OmpStructureChecker::Enter(const parser::OmpClause::Private &x) {
SymbolSourceMap symbols;
@@ -3347,6 +3348,7 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Private &x) {
CheckAllowedClause(llvm::omp::Clause::OMPC_private);
CheckIsVarPartOfAnotherVar(GetContext().clauseSource, x.v, "PRIVATE");
CheckIntentInPointer(symbols, llvm::omp::Clause::OMPC_private);
+ CheckCrayPointee(x.v, "PRIVATE");
}
void OmpStructureChecker::Enter(const parser::OmpClause::Nowait &x) {
@@ -3426,6 +3428,7 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Firstprivate &x) {
CheckAllowedClause(llvm::omp::Clause::OMPC_firstprivate);
CheckIsVarPartOfAnotherVar(GetContext().clauseSource, x.v, "FIRSTPRIVATE");
+ CheckCrayPointee(x.v, "FIRSTPRIVATE");
CheckIsLoopIvPartOfClause(llvmOmpClause::OMPC_firstprivate, x.v);
SymbolSourceMap currSymbols;
@@ -4522,6 +4525,22 @@ void OmpStructureChecker::CheckProcedurePointer(
}
}
+void OmpStructureChecker::CheckCrayPointee(
+ const parser::OmpObjectList &objectList, llvm::StringRef clause) {
+ SymbolSourceMap symbols;
+ GetSymbolsInObjectList(objectList, symbols);
+ for (auto it{symbols.begin()}; it != symbols.end(); ++it) {
+ const auto *symbol{it->first};
+ const auto source{it->second};
+ if (symbol->test(Symbol::Flag::CrayPointee)) {
+ context_.Say(source,
+ "Cray Pointee '%s' may not appear in %s clause, use Cray Pointer '%s' instead"_err_en_US,
+ symbol->name(), clause.str(),
+ semantics::GetCrayPointer(*symbol).name());
+ }
+ }
+}
+
void OmpStructureChecker::GetSymbolsInObjectList(
const parser::OmpObjectList &objectList, SymbolSourceMap &symbols) {
for (const auto &ompObject : objectList.v) {
diff --git a/flang/lib/Semantics/check-omp-structure.h b/flang/lib/Semantics/check-omp-structure.h
index 346a7bed9138f0..8bc37e98b3ce3b 100644
--- a/flang/lib/Semantics/check-omp-structure.h
+++ b/flang/lib/Semantics/check-omp-structure.h
@@ -194,6 +194,8 @@ class OmpStructureChecker
const parser::CharBlock &source, const parser::OmpObjectList &objList);
void CheckIntentInPointer(SymbolSourceMap &, const llvm::omp::Clause);
void CheckProcedurePointer(SymbolSourceMap &, const llvm::omp::Clause);
+ void CheckCrayPointee(
+ const parser::OmpObjectList &objectList, llvm::StringRef clause);
void GetSymbolsInObjectList(const parser::OmpObjectList &, SymbolSourceMap &);
void CheckDefinableObjects(SymbolSourceMap &, const llvm::omp::Clause);
void CheckCopyingPolymorphicAllocatable(
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 39478b58a9070d..9cbc61391ba1fb 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2115,8 +2115,12 @@ 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*/
+ (!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))) &&
!sym->owner().IsDerivedType() &&
sym->owner().kind() != Scope::Kind::ImpliedDos &&
!sym->detailsIf<semantics::AssocEntityDetails>() &&
@@ -2282,10 +2286,18 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
// 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)) {
- context_.Say(name.source,
- "The DEFAULT(NONE) clause requires that '%s' must be listed in "
- "a data-sharing attribute clause"_err_en_US,
- symbol->name());
+ if (symbol->test(Symbol::Flag::CrayPointee)) {
+ std::string crayPtrName{
+ semantics::GetCrayPointer(*symbol).name().ToString()};
+ if (!IsObjectWithDSA(*currScope().FindSymbol(crayPtrName)))
+ context_.Say(name.source,
+ "The DEFAULT(NONE) clause requires that the Cray Pointer '%s' must be listed in a data-sharing attribute clause"_err_en_US,
+ crayPtrName);
+ } else {
+ context_.Say(name.source,
+ "The DEFAULT(NONE) clause requires that '%s' must be listed in a data-sharing attribute clause"_err_en_US,
+ symbol->name());
+ }
}
}
}
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..c7d03f0db99040
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/cray-pointer-usage.f90
@@ -0,0 +1,27 @@
+!RUN: %python %S/../test_errors.py %s %flang -fopenmp
+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
|
|
Continuation of #82481 |
Thirumalai-Shaktivel
requested review from
mjklemm,
kiranktp,
tblah and
kparzysz
tblah
reviewed
Jan 2, 2025
Comment on lines
+4537
to
+4539
| "Cray Pointee '%s' may not appear in %s clause, use Cray Pointer '%s' instead"_err_en_US, | ||
| symbol->name(), clause.str(), | ||
| semantics::GetCrayPointer(*symbol).name()); |
There was a problem hiding this comment.
@mjklemm is this a correct interpretation of the standard? It isn't clear to me if naming cray pointees is actually prohibited, it only seems to require that the data-sharing attribute is the same as the pointer.
Of course, they are deprecated so we could disallow on those grounds (as we did with - reductions).
There was a problem hiding this comment.
The OpenMP specification is ambiguous on this. Let's do it this way and see if application users will complain about this. :-)
|
Thanks for the review! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/89/builds/14474 Here is the relevant piece of the build log for the reference |
I'm also seeing the same warning (treated as an error because of |
|
Yes, checking it. |
|
Fixed here: #123171 |
kiranchandramohan
added a commit
that referenced
this pull request
Jan 16, 2025
… DSA list" (#123220) Reverts #121028 Reverting due to CI failure (https://lab.llvm.org/buildbot/#/builders/89/builds/14474)
github-actions bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jan 16, 2025
…er usage in DSA list" (#123220) Reverts llvm/llvm-project#121028 Reverting due to CI failure (https://lab.llvm.org/buildbot/#/builders/89/builds/14474)
Thirumalai-Shaktivel
added a commit
that referenced
this pull request
Jan 28, 2025
Follow-up PR to fix the failure caused here: #121028 Failure: https://lab.llvm.org/buildbot/#/builders/89/builds/14474 Problems: - Cray pointee cannot be used in the DSA list (If used results in segmentation fault) - Cray pointer has to be in the DSA list when Cray pointee is used in the default (none) region Fix: Added required semantic checks along the tests Reference from the documentation (OpenMP 5.0: 2.19.1): - Cray pointees have the same data-sharing attribute as the storage with which their Cray pointers are associated.
github-actions bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jan 28, 2025
…list (#123171) Follow-up PR to fix the failure caused here: llvm/llvm-project#121028 Failure: https://lab.llvm.org/buildbot/#/builders/89/builds/14474 Problems: - Cray pointee cannot be used in the DSA list (If used results in segmentation fault) - Cray pointer has to be in the DSA list when Cray pointee is used in the default (none) region Fix: Added required semantic checks along the tests Reference from the documentation (OpenMP 5.0: 2.19.1): - Cray pointees have the same data-sharing attribute as the storage with which their Cray pointers are associated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problems:
Fix: Added required semantic checks along the tests
Reference from the documentation (OpenMP 5.0: 2.19.1):
which their Cray pointers are associated.