[flang][OpenMP] Properly resolve CRITICAL construct names#205904
Conversation
Resolve the names of CRITICAL constructs even if they are reserved names. Fixes #205855
llvmorg-github-actions
Bot
added
flang
|
@llvm/pr-subscribers-flang-parser @llvm/pr-subscribers-flang-semantics Author: Krzysztof Parzyszek (kparzysz) ChangesResolve the names of CRITICAL constructs even if they are reserved names. Fixes #205855 Full diff: https://github.com/llvm/llvm-project/pull/205904.diff 4 Files Affected:
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 061b2bdf775d0..fcd5e884db18d 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -649,7 +649,7 @@ void OmpStructureChecker::Enter(const parser::OmpLocator &x) {
if (auto *reserved{parser::Unwrap<parser::OmpReservedIdentifier>(x.u)}) {
std::string name{parser::ToLowerCaseLetters(reserved->v.source.ToString())};
if (!llvm::is_contained(llvm::omp::getReservedLocatorNames(), name)) {
- context_.Say(reserved->v.source, "'%s' is not a valid locator"_err_en_US,
+ context_.Say(reserved->v.source, "Invalid use of a reserved name '%s'"_err_en_US,
parser::ToUpperCaseLetters(name));
}
}
@@ -3099,6 +3099,11 @@ void OmpStructureChecker::Enter(const parser::OpenMPCriticalConstruct &x) {
if (auto *object{parser::Unwrap<parser::OmpObject>(arg.u)}) {
if (auto *designator{GetDesignatorFromObj(*object)}) {
return parser::GetDesignatorNameIfDataRef(*designator);
+ } else if (auto *locator{std::get_if<parser::OmpLocator>(&object->u)}) {
+ if (auto *res{
+ std::get_if<parser::OmpReservedIdentifier>(&locator->u)}) {
+ return &res->v;
+ }
}
}
return static_cast<const parser::Name *>(nullptr);
diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp
index 11e24a5e1cccb..8f7d5f0d3f959 100644
--- a/flang/lib/Semantics/resolve-names.cpp
+++ b/flang/lib/Semantics/resolve-names.cpp
@@ -2168,29 +2168,39 @@ void OmpVisitor::ProcessReductionSpecifier(
}
void OmpVisitor::ResolveCriticalName(const parser::OmpArgument &arg) {
- auto &globalScope{[&]() -> Scope & {
- for (Scope *s{&currScope()};; s = &s->parent()) {
- if (s->IsTopLevel()) {
- return *s;
- }
- }
- llvm_unreachable("Cannot find global scope");
- }()};
+ auto *object{parser::Unwrap<parser::OmpObject>(arg.u)};
+ if (!object) {
+ return;
+ }
- if (auto *object{parser::Unwrap<parser::OmpObject>(arg.u)}) {
- if (auto *desg{parser::omp::GetDesignatorFromObj(*object)}) {
- if (auto *name{parser::GetDesignatorNameIfDataRef(*desg)}) {
- if (auto *symbol{FindInScope(globalScope, *name)}) {
- if (!symbol->test(Symbol::Flag::OmpCriticalLock)) {
- SayWithDecl(*name, *symbol,
- "CRITICAL construct name '%s' conflicts with a previous declaration"_warn_en_US,
- name->ToString());
- }
- } else {
- name->symbol = &MakeSymbol(globalScope, name->source, Attrs{});
- name->symbol->set(Symbol::Flag::OmpCriticalLock);
- }
+ const parser::Name *name{common::visit( //
+ common::visitors{//
+ [&](const parser::Designator &x) -> const parser::Name * {
+ return parser::GetDesignatorNameIfDataRef(x);
+ },
+ [&](const parser::OmpLocator &x) -> const parser::Name * {
+ if (auto *res{std::get_if<parser::OmpReservedIdentifier>(&x.u)}) {
+ return &res->v;
+ }
+ return nullptr;
+ },
+ [&](const parser::Name &) -> const parser::Name * { return nullptr; },
+ [&](const parser::OmpObject::Invalid &) -> const parser::Name * {
+ return nullptr;
+ }},
+ object->u)};
+
+ if (name) {
+ if (auto *symbol{FindInScope(context().globalScope(), *name)}) {
+ if (!symbol->test(Symbol::Flag::OmpCriticalLock)) {
+ SayWithDecl(*name, *symbol,
+ "CRITICAL construct name '%s' conflicts with a previous declaration"_warn_en_US,
+ name->ToString());
}
+ } else {
+ name->symbol =
+ &MakeSymbol(context().globalScope(), name->source, Attrs{});
+ name->symbol->set(Symbol::Flag::OmpCriticalLock);
}
}
}
diff --git a/flang/test/Semantics/OpenMP/critical-reserved-name.f90 b/flang/test/Semantics/OpenMP/critical-reserved-name.f90
new file mode 100644
index 0000000000000..9516f0c07d7b7
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/critical-reserved-name.f90
@@ -0,0 +1,8 @@
+! RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp
+
+subroutine f
+ !ERROR: Invalid use of a reserved name 'OMP_C'
+ !$omp critical(omp_c)
+ !ERROR: Invalid use of a reserved name 'OMP_C'
+ !$omp end critical(omp_c)
+end
diff --git a/flang/test/Semantics/OpenMP/reserved-locator.f90 b/flang/test/Semantics/OpenMP/reserved-locator.f90
index 3fc45ffa0f54c..ae2a6d59f4c74 100644
--- a/flang/test/Semantics/OpenMP/reserved-locator.f90
+++ b/flang/test/Semantics/OpenMP/reserved-locator.f90
@@ -1,6 +1,6 @@
!RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp -fopenmp-version=60
subroutine f
-!ERROR: 'OMP_SOME_MEMORY' is not a valid locator
+!ERROR: Invalid use of a reserved name 'OMP_SOME_MEMORY'
!$omp target update from(omp_some_memory)
end
|
|
@llvm/pr-subscribers-flang-openmp Author: Krzysztof Parzyszek (kparzysz) ChangesResolve the names of CRITICAL constructs even if they are reserved names. Fixes #205855 Full diff: https://github.com/llvm/llvm-project/pull/205904.diff 4 Files Affected:
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 061b2bdf775d0..fcd5e884db18d 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -649,7 +649,7 @@ void OmpStructureChecker::Enter(const parser::OmpLocator &x) {
if (auto *reserved{parser::Unwrap<parser::OmpReservedIdentifier>(x.u)}) {
std::string name{parser::ToLowerCaseLetters(reserved->v.source.ToString())};
if (!llvm::is_contained(llvm::omp::getReservedLocatorNames(), name)) {
- context_.Say(reserved->v.source, "'%s' is not a valid locator"_err_en_US,
+ context_.Say(reserved->v.source, "Invalid use of a reserved name '%s'"_err_en_US,
parser::ToUpperCaseLetters(name));
}
}
@@ -3099,6 +3099,11 @@ void OmpStructureChecker::Enter(const parser::OpenMPCriticalConstruct &x) {
if (auto *object{parser::Unwrap<parser::OmpObject>(arg.u)}) {
if (auto *designator{GetDesignatorFromObj(*object)}) {
return parser::GetDesignatorNameIfDataRef(*designator);
+ } else if (auto *locator{std::get_if<parser::OmpLocator>(&object->u)}) {
+ if (auto *res{
+ std::get_if<parser::OmpReservedIdentifier>(&locator->u)}) {
+ return &res->v;
+ }
}
}
return static_cast<const parser::Name *>(nullptr);
diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp
index 11e24a5e1cccb..8f7d5f0d3f959 100644
--- a/flang/lib/Semantics/resolve-names.cpp
+++ b/flang/lib/Semantics/resolve-names.cpp
@@ -2168,29 +2168,39 @@ void OmpVisitor::ProcessReductionSpecifier(
}
void OmpVisitor::ResolveCriticalName(const parser::OmpArgument &arg) {
- auto &globalScope{[&]() -> Scope & {
- for (Scope *s{&currScope()};; s = &s->parent()) {
- if (s->IsTopLevel()) {
- return *s;
- }
- }
- llvm_unreachable("Cannot find global scope");
- }()};
+ auto *object{parser::Unwrap<parser::OmpObject>(arg.u)};
+ if (!object) {
+ return;
+ }
- if (auto *object{parser::Unwrap<parser::OmpObject>(arg.u)}) {
- if (auto *desg{parser::omp::GetDesignatorFromObj(*object)}) {
- if (auto *name{parser::GetDesignatorNameIfDataRef(*desg)}) {
- if (auto *symbol{FindInScope(globalScope, *name)}) {
- if (!symbol->test(Symbol::Flag::OmpCriticalLock)) {
- SayWithDecl(*name, *symbol,
- "CRITICAL construct name '%s' conflicts with a previous declaration"_warn_en_US,
- name->ToString());
- }
- } else {
- name->symbol = &MakeSymbol(globalScope, name->source, Attrs{});
- name->symbol->set(Symbol::Flag::OmpCriticalLock);
- }
+ const parser::Name *name{common::visit( //
+ common::visitors{//
+ [&](const parser::Designator &x) -> const parser::Name * {
+ return parser::GetDesignatorNameIfDataRef(x);
+ },
+ [&](const parser::OmpLocator &x) -> const parser::Name * {
+ if (auto *res{std::get_if<parser::OmpReservedIdentifier>(&x.u)}) {
+ return &res->v;
+ }
+ return nullptr;
+ },
+ [&](const parser::Name &) -> const parser::Name * { return nullptr; },
+ [&](const parser::OmpObject::Invalid &) -> const parser::Name * {
+ return nullptr;
+ }},
+ object->u)};
+
+ if (name) {
+ if (auto *symbol{FindInScope(context().globalScope(), *name)}) {
+ if (!symbol->test(Symbol::Flag::OmpCriticalLock)) {
+ SayWithDecl(*name, *symbol,
+ "CRITICAL construct name '%s' conflicts with a previous declaration"_warn_en_US,
+ name->ToString());
}
+ } else {
+ name->symbol =
+ &MakeSymbol(context().globalScope(), name->source, Attrs{});
+ name->symbol->set(Symbol::Flag::OmpCriticalLock);
}
}
}
diff --git a/flang/test/Semantics/OpenMP/critical-reserved-name.f90 b/flang/test/Semantics/OpenMP/critical-reserved-name.f90
new file mode 100644
index 0000000000000..9516f0c07d7b7
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/critical-reserved-name.f90
@@ -0,0 +1,8 @@
+! RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp
+
+subroutine f
+ !ERROR: Invalid use of a reserved name 'OMP_C'
+ !$omp critical(omp_c)
+ !ERROR: Invalid use of a reserved name 'OMP_C'
+ !$omp end critical(omp_c)
+end
diff --git a/flang/test/Semantics/OpenMP/reserved-locator.f90 b/flang/test/Semantics/OpenMP/reserved-locator.f90
index 3fc45ffa0f54c..ae2a6d59f4c74 100644
--- a/flang/test/Semantics/OpenMP/reserved-locator.f90
+++ b/flang/test/Semantics/OpenMP/reserved-locator.f90
@@ -1,6 +1,6 @@
!RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp -fopenmp-version=60
subroutine f
-!ERROR: 'OMP_SOME_MEMORY' is not a valid locator
+!ERROR: Invalid use of a reserved name 'OMP_SOME_MEMORY'
!$omp target update from(omp_some_memory)
end
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
|
This looks correct to me, and the cleanup is a real improvement. Replacing the previous One optional consistency/severity question, not a blocker: OpenMP 5.2 §4 does reserve names beginning with Given that, and given the compatibility concern in #205855, do we intentionally want this as a hard error here, or would a warning/portability diagnostic be a better fit until reserved-name checking is applied more uniformly? The existing |
|
I strongly recommend that these kinds of errors should be a warning, so that the user could promote them to errors or turn them off, depending on their needs. This behavior could be documented as OpenMP extension. |
The original fix actually did that, but since I had to make changes to lowering to handle reserved identifiers where only a designator was expected before, I was worried that allowing this could open some unforeseen can of worms. I can come up with a solution that improves how we deal with reserved names (and emits a warning instead) if you're willing to wait until next week. |
|
This is a quick fix that ignores reserved names except for predefined locator names. It won't emit any diagnostics for the testcase in the issue, but a future PR will add those as warnings. |
|
Thanks for reworking this. The new direction looks good to me. One small request before I approve: since the previous reserved-name tests were removed when the behavior changed, could we keep/add a direct regression test showing that |
|
Thanks for the quick action! We are seeing similar errors pop up in a few more contexts in real applications and in lots of test cases. @scamp-nvidia will file a few more issues to track in case you'd like to expand this PR. We're committed to informing our upstream users and modifying our tests to comply with the OpenMP standard but the quick relief proposed in this PR is very welcome! |
I'd rather not add a test that checks for behavior that's strictly speaking incorrect. I'll add checks for diagnostics in the PR that shows them. |
|
I'm going to merge this, since it fixes the biggest problem. |
Resolve the names of CRITICAL constructs even if they are reserved names. This also limits locator parsing to known reserved names. Fixes llvm#205855
Resolve the names of CRITICAL constructs even if they are reserved names. This also limits locator parsing to known reserved names. Fixes llvm#205855
4 participants
Resolve the names of CRITICAL constructs even if they are reserved names.
This also limits locator parsing to known reserved names.
Fixes #205855