[flang][OpenMP] Allow flush of common block #139528
Merged
Conversation
This file contains hidden or 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
llvmbot
added
flang
Member
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-flang-openmp Author: Tom Eccles (tblah) ChangesI think this was denied by accident in 68180d8. Flush of a common block is allowed by the standard on my reading. It is This doesn't need any lowering changes. The LLVM translation ignores the Depends upon #139522. Ignore the first commit in this PR. Full diff: https://github.com/llvm/llvm-project/pull/139528.diff 3 Files Affected:
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index f17de42ca2466..591f30db36baa 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -2304,13 +2304,6 @@ void OmpStructureChecker::Leave(const parser::OpenMPFlushConstruct &x) {
auto &flushList{std::get<std::optional<parser::OmpArgumentList>>(x.v.t)};
if (flushList) {
- for (const parser::OmpArgument &arg : flushList->v) {
- if (auto *sym{GetArgumentSymbol(arg)}; sym && !IsVariableListItem(*sym)) {
- context_.Say(arg.source,
- "FLUSH argument must be a variable list item"_err_en_US);
- }
- }
-
if (FindClause(llvm::omp::Clause::OMPC_acquire) ||
FindClause(llvm::omp::Clause::OMPC_release) ||
FindClause(llvm::omp::Clause::OMPC_acq_rel)) {
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 8b1caca34a6a7..68f8cf9f17620 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -409,6 +409,19 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
}
void Post(const parser::OpenMPDepobjConstruct &) { PopContext(); }
+ bool Pre(const parser::OpenMPFlushConstruct &x) {
+ PushContext(x.source, llvm::omp::Directive::OMPD_flush);
+ for (auto &arg : x.v.Arguments().v) {
+ if (auto *locator{std::get_if<parser::OmpLocator>(&arg.u)}) {
+ if (auto *object{std::get_if<parser::OmpObject>(&locator->u)}) {
+ ResolveOmpObject(*object, Symbol::Flag::OmpDependObject);
+ }
+ }
+ }
+ return true;
+ }
+ void Post(const parser::OpenMPFlushConstruct &) { PopContext(); }
+
bool Pre(const parser::OpenMPRequiresConstruct &x) {
using Flags = WithOmpDeclarative::RequiresFlags;
using Requires = WithOmpDeclarative::RequiresFlag;
diff --git a/flang/test/Lower/OpenMP/flush-common.f90 b/flang/test/Lower/OpenMP/flush-common.f90
new file mode 100644
index 0000000000000..7656141dcb295
--- /dev/null
+++ b/flang/test/Lower/OpenMP/flush-common.f90
@@ -0,0 +1,13 @@
+! RUN: %flang_fc1 -fopenmp -emit-hlfir -o - %s | FileCheck %s
+
+! Regression test to ensure that the name /c/ in the flush argument list is
+! resolved to the common block symbol and common blocks are allowed in the
+! flush argument list.
+
+! CHECK: %[[GLBL:.*]] = fir.address_of({{.*}}) : !fir.ref<!fir.array<4xi8>>
+ common /c/ x
+ real :: x
+! CHECK: omp.flush(%[[GLBL]] : !fir.ref<!fir.array<4xi8>>)
+ !$omp flush(/c/)
+end
+
|
Member
|
@llvm/pr-subscribers-flang-semantics Author: Tom Eccles (tblah) ChangesI think this was denied by accident in 68180d8. Flush of a common block is allowed by the standard on my reading. It is This doesn't need any lowering changes. The LLVM translation ignores the Depends upon #139522. Ignore the first commit in this PR. Full diff: https://github.com/llvm/llvm-project/pull/139528.diff 3 Files Affected:
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index f17de42ca2466..591f30db36baa 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -2304,13 +2304,6 @@ void OmpStructureChecker::Leave(const parser::OpenMPFlushConstruct &x) {
auto &flushList{std::get<std::optional<parser::OmpArgumentList>>(x.v.t)};
if (flushList) {
- for (const parser::OmpArgument &arg : flushList->v) {
- if (auto *sym{GetArgumentSymbol(arg)}; sym && !IsVariableListItem(*sym)) {
- context_.Say(arg.source,
- "FLUSH argument must be a variable list item"_err_en_US);
- }
- }
-
if (FindClause(llvm::omp::Clause::OMPC_acquire) ||
FindClause(llvm::omp::Clause::OMPC_release) ||
FindClause(llvm::omp::Clause::OMPC_acq_rel)) {
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 8b1caca34a6a7..68f8cf9f17620 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -409,6 +409,19 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
}
void Post(const parser::OpenMPDepobjConstruct &) { PopContext(); }
+ bool Pre(const parser::OpenMPFlushConstruct &x) {
+ PushContext(x.source, llvm::omp::Directive::OMPD_flush);
+ for (auto &arg : x.v.Arguments().v) {
+ if (auto *locator{std::get_if<parser::OmpLocator>(&arg.u)}) {
+ if (auto *object{std::get_if<parser::OmpObject>(&locator->u)}) {
+ ResolveOmpObject(*object, Symbol::Flag::OmpDependObject);
+ }
+ }
+ }
+ return true;
+ }
+ void Post(const parser::OpenMPFlushConstruct &) { PopContext(); }
+
bool Pre(const parser::OpenMPRequiresConstruct &x) {
using Flags = WithOmpDeclarative::RequiresFlags;
using Requires = WithOmpDeclarative::RequiresFlag;
diff --git a/flang/test/Lower/OpenMP/flush-common.f90 b/flang/test/Lower/OpenMP/flush-common.f90
new file mode 100644
index 0000000000000..7656141dcb295
--- /dev/null
+++ b/flang/test/Lower/OpenMP/flush-common.f90
@@ -0,0 +1,13 @@
+! RUN: %flang_fc1 -fopenmp -emit-hlfir -o - %s | FileCheck %s
+
+! Regression test to ensure that the name /c/ in the flush argument list is
+! resolved to the common block symbol and common blocks are allowed in the
+! flush argument list.
+
+! CHECK: %[[GLBL:.*]] = fir.address_of({{.*}}) : !fir.ref<!fir.array<4xi8>>
+ common /c/ x
+ real :: x
+! CHECK: omp.flush(%[[GLBL]] : !fir.ref<!fir.array<4xi8>>)
+ !$omp flush(/c/)
+end
+
|
Contributor
|
Thanks for the patch Tom. LGTM |
I think this was denied by accident in 68180d8. Flush of a common block is allowed by the standard on my reading. It is not allowed by classic-flang but is supported by gfortran and ifx. This doesn't need any lowering changes. The LLVM translation ignores the flush argument list because the openmp runtime library doesn't support flushing specific data. Depends upon llvm#139522. Ignore the first commit in this PR.
tblah
force-pushed
the
ecclescake/flush-common2
branch
from
890a8a0 to
3e6d5c3
Compare
|
|
||
| if (flushList) { | ||
| for (const parser::OmpArgument &arg : flushList->v) { | ||
| if (auto *sym{GetArgumentSymbol(arg)}; sym && !IsVariableListItem(*sym)) { |
Contributor
There was a problem hiding this comment.
@kparzysz is this an issue with the function IsVariableListItem?
A variable list item is one of the following:
2 • a variable that is not coindexed and that is not a substring;
3 • an array section that is not coindexed and that does not contain an element that is a substring;
4 • a named constant;
5 • an associate name that may appear in a variable definition context; or
6 • a common block name (enclosed in slashes).
Contributor
There was a problem hiding this comment.
I wonder whether the following is the correct fix here.
bool OmpStructureChecker::IsVariableListItem(const Symbol &sym) {
- return evaluate::IsVariable(sym) || sym.attrs().test(Attr::POINTER);
+ return evaluate::IsVariable(sym) || sym.attrs().test(Attr::POINTER) ||
+ sym.detailsIf<semantics::CommonBlockDetails>();
}
Contributor
Author
There was a problem hiding this comment.
My worry was whether that would affect many other constructs. Especially through this use:
void OmpStructureChecker::Enter(const parser::OmpClause &x)Even if it is wrong that those constructs currently do not accept common blocks, it is unlikely that they will all generate correct code in this case.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
I think this was denied by accident in 68180d8.
Flush of a common block is allowed by the standard on my reading. It is
not allowed by classic-flang but is supported by gfortran and ifx.
This doesn't need any lowering changes. The LLVM translation ignores the
flush argument list because the openmp runtime library doesn't support
flushing specific data.
Depends upon #139522. Ignore the first commit in this PR.