[flang][OpenMP][Semantics] resolve objects in the flush arg list #139522
Conversation
Fixes llvm#136583 Normally the flush argument list would contain a DataRef to some variable. All DataRefs are handled generically in resolve-names and so the problem wasn't observed. But when a common block name is specified, this is not parsed as a DataRef. There was already handling in resolve-directives for OmpObjectList but not for argument lists. I've added a visitor for FLUSH which ensures all of the arguments have been resolved. The test is there to make sure the compiler doesn't crashed encountering the unresolved symbol. It shows that we currently deny flushing a common block. I'm not sure that it is right to restrict common blocks from flush argument lists, but fixing that can come in a different patch. This one is fixing an ICE.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-flang-semantics Author: Tom Eccles (tblah) ChangesFixes #136583 Normally the flush argument list would contain a DataRef to some variable. All DataRefs are handled generically in resolve-names and so the problem wasn't observed. But when a common block name is specified, this is not parsed as a DataRef. There was already handling in resolve-directives for OmpObjectList but not for argument lists. I've added a visitor for FLUSH which ensures all of the arguments have been resolved. The test is there to make sure the compiler doesn't crashed encountering the unresolved symbol. It shows that we currently deny flushing a common block. I'm not sure that it is right to restrict common blocks from flush argument lists, but fixing that can come in a different patch. This one is fixing an ICE. Full diff: https://github.com/llvm/llvm-project/pull/139522.diff 2 Files Affected:
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/Semantics/OpenMP/flush04.f90 b/flang/test/Semantics/OpenMP/flush04.f90
new file mode 100644
index 0000000000000..ffc2273b692dc
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/flush04.f90
@@ -0,0 +1,11 @@
+! RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp
+
+! Regression test to ensure that the name /c/ in the flush argument list is
+! resolved to the common block symbol.
+
+ common /c/ x
+ real :: x
+!ERROR: FLUSH argument must be a variable list item
+ !$omp flush(/c/)
+end
+
|
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.
| 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); |
There was a problem hiding this comment.
Assuming this is a copy paste error. This is not a DependObject, so I think adding the Symbol::Flag::OmpDependObject is probably not right.
What do we want to do here? Is it just resolving the names in the flush argument list to the appropriate Fortran declarations? If so, we should use ResolveName.
There was a problem hiding this comment.
If common blocks are not handled in ResolveName then you can probably use OmpFlushed as the Symbol in ResolveOmpObject.
There was a problem hiding this comment.
Yes it was a copy and paste error. Thanks for catching this.
I had assumed that given an object, the right way to resolve it was to call ResolveOmpObject. I have now changed to include only the relevant parts of the code inline.
|
The CI failure looks unrelated. Thanks for the quick review. |
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.
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.
I think this was denied by accident in llvm/llvm-project@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/llvm-project#139522. Ignore the first commit in this PR.
I think this was denied by accident in llvm@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.
Fixes #136583
Normally the flush argument list would contain a DataRef to some variable. All DataRefs are handled generically in resolve-names and so the problem wasn't observed. But when a common block name is specified, this is not parsed as a DataRef. There was already handling in resolve-directives for OmpObjectList but not for argument lists. I've added a visitor for FLUSH which ensures all of the arguments have been resolved.
The test is there to make sure the compiler doesn't crashed encountering the unresolved symbol. It shows that we currently deny flushing a common block. I'm not sure that it is right to restrict common blocks from flush argument lists, but fixing that can come in a different patch. This one is fixing an ICE.