[flang][OpenACC] Relax COMMON block usage restriction in OpenACC directives #162659
Conversation
for ACC directives.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-semantics @llvm/pr-subscribers-openacc Author: Eugene Epshteyn (eugeneepshteyn) ChangesUnlike OpenMP, OpenACC doesn't require that the COMMON block be defined in the same scope as the directive. Full diff: https://github.com/llvm/llvm-project/pull/162659.diff 2 Files Affected:
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 18fc63814d973..44cfb75e06164 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -1695,17 +1695,23 @@ void AccAttributeVisitor::Post(const parser::Name &name) {
Symbol *AccAttributeVisitor::ResolveAccCommonBlockName(
const parser::Name *name) {
- if (auto *prev{name
- ? GetContext().scope.parent().FindCommonBlock(name->source)
- : nullptr}) {
- name->symbol = prev;
- return prev;
- }
- // Check if the Common Block is declared in the current scope
- if (auto *commonBlockSymbol{
- name ? GetContext().scope.FindCommonBlock(name->source) : nullptr}) {
- name->symbol = commonBlockSymbol;
- return commonBlockSymbol;
+ if (!name) {
+ return nullptr;
+ }
+ // Check the local and surrounding scopes first
+ if (auto *cb{GetProgramUnitOrBlockConstructContaining(GetContext().scope)
+ .FindCommonBlock(name->source)}) {
+ name->symbol = cb;
+ return cb;
+ }
+ // Look for COMMON block in the modules
+ for (const Scope &childScope : context_.globalScope().children()) {
+ if (childScope.kind() == Scope::Kind::Module) {
+ if (auto *cb{childScope.FindCommonBlock(name->source)}) {
+ name->symbol = cb;
+ return cb;
+ }
+ }
}
return nullptr;
}
@@ -1755,8 +1761,8 @@ void AccAttributeVisitor::ResolveAccObject(
}
} else {
context_.Say(name.source,
- "COMMON block must be declared in the same scoping unit "
- "in which the OpenACC directive or clause appears"_err_en_US);
+ "Could not find COMMON block '%s' used in OpenACC directive"_err_en_US,
+ name.ToString());
}
},
},
diff --git a/flang/test/Semantics/OpenACC/acc-common.f90 b/flang/test/Semantics/OpenACC/acc-common.f90
new file mode 100644
index 0000000000000..c6142bdb915f6
--- /dev/null
+++ b/flang/test/Semantics/OpenACC/acc-common.f90
@@ -0,0 +1,19 @@
+! RUN: %python %S/../test_errors.py %s %flang -fopenacc
+module acc_common_decl
+ implicit none
+ integer a
+ common /a_common/ a
+!$acc declare create (/a_common/)
+ data a/42/
+end module acc_common_decl
+
+program acc_decl_test
+ use acc_common_decl
+ implicit none
+
+ a = 1
+!$acc update device (/a_common/)
+ a = 2
+!ERROR: Could not find COMMON block 'a_common_bad' used in OpenACC directive
+!$acc update device (/a_common_bad/)
+end program
|
| name->symbol = cb; | ||
| return cb; | ||
| } | ||
| // Look for COMMON block in the modules |
There was a problem hiding this comment.
Are you sure that this part makes sense? Do you have an example where NVF looks at non-USE-associated modules? You probably just want to look for a USE-associated common block in the enclosing scopes instead.
There was a problem hiding this comment.
It would have been very convenient to have a USE-associated common block available, but I don't see one created. With this program
module acc_decl
implicit none
integer a
common /a_common/ a
!$acc declare create (/a_common/)
data a/42/
end module acc_decl
program acc_decl_test
use acc_decl
implicit none
a = 1
!$acc update device (/a_common/)
a = 2
end program
... and -fdebug-dump-symbols I only see
MainProgram scope: ACC_DECL_TEST size=0 alignment=1 sourceRange=72 bytes
a (InDataStmt, InCommonBlock): Use from a in acc_decl
Perhaps this is the problem? Perhaps we should be creating USE-associated symbol from its use in !$acc update device (/a_common/)?
| if (!name) { | ||
| return nullptr; | ||
| } | ||
| // Check the local and surrounding scopes first |
There was a problem hiding this comment.
This will only check the innermost BLOCK construct or subprogram, not any containing construct or host subprogram.
…. (Currently the code doesn't fix ACC COMMON problem.)
…This resolves OpenMP regressions with COMMON. Still working on USE-association for COMMON blocks
|
|
||
| /// Find COMMON block in current and surrounding scopes, follow USE | ||
| /// associations | ||
| Symbol *FindCommonBlockInScopes(const SourceName &) const; |
There was a problem hiding this comment.
The name could be more precise so that it's immediately understandable at call sites.
…ent scope and COMMON with USE details
|
|
||
| // Find COMMON block in current and surrounding scopes, follow USE | ||
| // associations | ||
| Symbol *FindCommonBlockInSurroundingScopes(const SourceName &) const; |
There was a problem hiding this comment.
The name isn't consistent with the comment; will this member function detect a common block in this scope, or only in its parents?
There was a problem hiding this comment.
I would appreciate suggestion for a better name, otherwise it'll be FindCommonBlockInCurrentAndSurroundingScopes() :-)
There was a problem hiding this comment.
Maybe FindCommonBlockInLexicalScopes or VisibleScopes.
There was a problem hiding this comment.
I went with FindCommonBlockInVisibleScopes()
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| Symbol &MakeCommonBlock(SourceName, SourceName location); | ||
| Symbol *FindCommonBlock(const SourceName &) const; | ||
| bool AddCommonBlockUse(const SourceName &name, Symbol &cbSymbol); | ||
| mapType &commonBlockUses() { return commonBlockUses_; } |
There was a problem hiding this comment.
Consider adding a const accessor as well, and moving this up to be with the other accessors.
| name ? GetContext().scope.FindCommonBlock(name->source) : nullptr}) { | ||
| name->symbol = commonBlockSymbol; | ||
| return commonBlockSymbol; | ||
| if (!name) { |
There was a problem hiding this comment.
This would be more clear as if (name) { ..., I think.
| if (!name) { | ||
| return nullptr; | ||
| } | ||
| if (auto *cb{ |
There was a problem hiding this comment.
Please use const for pointers and references that are auto, and maybe const Symbol * would be even clearer.
There was a problem hiding this comment.
I made it Symbol *, but cannot make it const, because it's assigned to non-const below.
| if (!currScope().FindCommonBlockInVisibleScopes(name)) { | ||
| // Make a symbol, but don't add it to the Scope, since it needs to | ||
| // be added to the USE-associated COMMON blocks | ||
| Symbol *localCB{&currScope().MakeSymbol( |
There was a problem hiding this comment.
This variable doesn't have to be a pointer. MakeSymbol returns a reference, and you can declare localCB to be a reference that you can then just pass directly to AddCommonBlockUse as such.
| } | ||
| } | ||
| } | ||
| // Go through the list of COMMON block symbols in the module scope and add |
There was a problem hiding this comment.
What happens when the same COMMON block name arrives from multiple modules?
There was a problem hiding this comment.
The first use will be added via AddCommonBlockUse(), but adding the second use will silently fail. What should be the behavior in such case?
There was a problem hiding this comment.
I don't know. Maybe you would want to retain the largest definition of the COMMON block, and perhaps warn if their sizes differ.
There was a problem hiding this comment.
COMMON block names and sizes are already stored in SemanticsContext, available via SemanticsContext::GetCommonBlocks(). This is what lowering is using to generate correctly sized blob for COMMON.
We already have a warning for COMMON size differences. The following code
module common_multi_mod_a
integer :: a
common /my_common/ a
end module
module common_multi_mod_b
integer :: b
integer :: c
common /my_common/ b, c
end module
subroutine s1()
use common_multi_mod_a
a = 1
end subroutine
subroutine s2()
use common_multi_mod_b
b = 2
c = 3
end subroutine
subroutine s3()
use common_multi_mod_b
use common_multi_mod_a
a = 4
b = 5
c = 6
end subroutine
... generates the following warning:
$ flang -c common-multi-mod.f90 -pedantic
./common-multi-mod.f90:9:11: portability: A named COMMON block should have the same size everywhere it appears (8 bytes here) [-Wdistinct-common-sizes]
common /my_common/ b, c
^^^^^^^^^
./common-multi-mod.f90:3:11: Previously defined with a size of 4 bytes
common /my_common/ a
^^^^^^^^^
There was a problem hiding this comment.
(Note that a and b in the example above occupy the same space. We don't warn about that. Should we?)
There was a problem hiding this comment.
No, it's storage association.
There was a problem hiding this comment.
Maybe you would want to retain the largest definition of the COMMON block
I implemented that, but unfortunately in PerformStatementSemantics(), ResolveNames() is called before ComputeOffsets(), so don't know COMMON block sizes at the time of the name resolution. I undid my implementation, but kept AddCommonBlockUse() change
…lock in case of uses from multiple modules, except that it doesn't work: COMMON block sizes are computed after the name resolution
…lock in case of uses from multiple modules, except that it doesn't work: COMMON block sizes are computed after the name resolution
4 participants
Unlike OpenMP, OpenACC doesn't require that the COMMON block be defined in the same scope as the directive.