Skip to content

Commit eb9bf18

Browse files
luporlluporl
authored
[flang][OpenMP] Fix copyprivate semantic checks (llvm#95799)
There are some cases in which variables used in OpenMP constructs are predetermined as private. The semantic checks for copyprivate were not handling those cases. Besides that, shared symbols were not being properly represented in some cases. When there was no previously declared private (implicit) symbol, no new association symbols, representing shared ones, were being created. These symbols must always be inserted in constructs that may privatize the original symbol: parallel, teams and task generating constructs. Fixes llvm#87214 and llvm#86907
1 parent 8ebe499 commit eb9bf18

File tree

15 files changed

+250
-95
lines changed

15 files changed

+250
-95
lines changed

‎flang/include/flang/Semantics/tools.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ bool IsIntrinsicConcat(
8787
bool IsGenericDefinedOp(const Symbol &);
8888
bool IsDefinedOperator(SourceName);
8989
std::string MakeOpName(SourceName);
90+
bool IsCommonBlockContaining(const Symbol &, const Symbol &);
9091

9192
// Returns true if maybeAncestor exists and is a proper ancestor of a
9293
// descendent scope (or symbol owner). Will be false, unlike Scope::Contains(),

‎flang/lib/Semantics/resolve-directives.cpp‎

Lines changed: 90 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include "flang/Parser/parse-tree.h"
2020
#include "flang/Parser/tools.h"
2121
#include "flang/Semantics/expression.h"
22+
#include "flang/Semantics/tools.h"
2223
#include <list>
2324
#include <map>
2425
#include <sstream>
@@ -729,7 +730,6 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
729730
void CheckNameInAllocateStmt(const parser::CharBlock &source,
730731
const parser::Name &ompObject, const parser::AllocateStmt &allocate);
731732

732-
bool HasSymbolInEnclosingScope(const Symbol &, Scope &);
733733
std::int64_t ordCollapseLevel{0};
734734

735735
void AddOmpRequiresToScope(Scope &, WithOmpDeclarative::RequiresFlags,
@@ -2085,16 +2085,22 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
20852085
}
20862086
}
20872087

2088-
// When handling each implicit rule, either a new private symbol is
2089-
// declared or the last declared symbol is used.
2090-
// In the latter case, it's necessary to insert a new symbol in the scope
2091-
// being processed, associated with the last declared symbol.
2092-
// This captures the fact that, although we are using the last declared
2093-
// symbol, its DSA could be different in this scope.
2094-
// Also, because of how symbols are collected in lowering, not inserting
2095-
// a new symbol in this scope could lead to the conclusion that the
2096-
// symbol was declared in this construct, which would result in wrong
2097-
// privatization code being generated.
2088+
// When handling each implicit rule for a given symbol, one of the
2089+
// following 3 actions may be taken:
2090+
// 1. Declare a new private symbol.
2091+
// 2. Create a new association symbol with no flags, that will represent
2092+
// a shared symbol in the current scope. Note that symbols without
2093+
// any private flags are considered as shared.
2094+
// 3. Use the last declared private symbol, by inserting a new symbol
2095+
// in the scope being processed, associated with it.
2096+
// If no private symbol was declared previously, then no association
2097+
// is needed and the symbol from the enclosing scope will be
2098+
// inherited by the current one.
2099+
//
2100+
// Because of how symbols are collected in lowering, not inserting a new
2101+
// symbol in the last case could lead to the conclusion that a symbol
2102+
// from an enclosing construct was declared in the current construct,
2103+
// which would result in wrong privatization code being generated.
20982104
// Consider the following example:
20992105
//
21002106
// !$omp parallel default(private) ! p1
@@ -2107,48 +2113,56 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
21072113
// (p2), it would use the x symbol definition from the enclosing scope.
21082114
// Then, when p2's default symbols were collected in lowering, the x
21092115
// symbol from the outer parallel construct (p1) would be collected, as
2110-
// it would have the private flag set (note that symbols that don't have
2111-
// any private flag are considered as shared).
2116+
// it would have the private flag set.
21122117
// This would make x appear to be defined in p2, causing it to be
21132118
// privatized in p2 and its privatization in p1 to be skipped.
2114-
auto declNewSymbol = [&](Symbol::Flag flag) {
2119+
auto makePrivateSymbol = [&](Symbol::Flag flag) {
21152120
Symbol *hostSymbol =
21162121
lastDeclSymbol ? lastDeclSymbol : &symbol->GetUltimate();
21172122
lastDeclSymbol = DeclarePrivateAccessEntity(
21182123
*hostSymbol, flag, context_.FindScope(dirContext.directiveSource));
21192124
return lastDeclSymbol;
21202125
};
2126+
auto makeSharedSymbol = [&]() {
2127+
Symbol *hostSymbol =
2128+
lastDeclSymbol ? lastDeclSymbol : &symbol->GetUltimate();
2129+
MakeAssocSymbol(symbol->name(), *hostSymbol,
2130+
context_.FindScope(dirContext.directiveSource));
2131+
};
21212132
auto useLastDeclSymbol = [&]() {
21222133
if (lastDeclSymbol)
21232134
MakeAssocSymbol(symbol->name(), *lastDeclSymbol,
21242135
context_.FindScope(dirContext.directiveSource));
21252136
};
21262137

2138+
bool taskGenDir = llvm::omp::taskGeneratingSet.test(dirContext.directive);
2139+
bool targetDir = llvm::omp::allTargetSet.test(dirContext.directive);
2140+
bool parallelDir = llvm::omp::allParallelSet.test(dirContext.directive);
2141+
bool teamsDir = llvm::omp::allTeamsSet.test(dirContext.directive);
2142+
21272143
if (dsa.has_value()) {
2128-
useLastDeclSymbol();
2144+
if (dsa.value() == Symbol::Flag::OmpShared &&
2145+
(parallelDir || taskGenDir || teamsDir))
2146+
makeSharedSymbol();
2147+
// Private symbols will have been declared already.
21292148
prevDSA = dsa;
21302149
continue;
21312150
}
21322151

2133-
bool taskGenDir = llvm::omp::taskGeneratingSet.test(dirContext.directive);
2134-
bool targetDir = llvm::omp::allTargetSet.test(dirContext.directive);
2135-
bool parallelDir = llvm::omp::allParallelSet.test(dirContext.directive);
2136-
21372152
if (dirContext.defaultDSA == Symbol::Flag::OmpPrivate ||
21382153
dirContext.defaultDSA == Symbol::Flag::OmpFirstPrivate ||
21392154
dirContext.defaultDSA == Symbol::Flag::OmpShared) {
21402155
// 1) default
21412156
// Allowed only with parallel, teams and task generating constructs.
2142-
assert(parallelDir || taskGenDir ||
2143-
llvm::omp::allTeamsSet.test(dirContext.directive));
2157+
assert(parallelDir || taskGenDir || teamsDir);
21442158
if (dirContext.defaultDSA != Symbol::Flag::OmpShared)
2145-
declNewSymbol(dirContext.defaultDSA);
2159+
makePrivateSymbol(dirContext.defaultDSA);
21462160
else
2147-
useLastDeclSymbol();
2161+
makeSharedSymbol();
21482162
dsa = dirContext.defaultDSA;
21492163
} else if (parallelDir) {
21502164
// 2) parallel -> shared
2151-
useLastDeclSymbol();
2165+
makeSharedSymbol();
21522166
dsa = Symbol::Flag::OmpShared;
21532167
} else if (!taskGenDir && !targetDir) {
21542168
// 3) enclosing context
@@ -2161,12 +2175,12 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
21612175
// TODO 5) dummy arg in orphaned taskgen construct -> firstprivate
21622176
if (prevDSA == Symbol::Flag::OmpShared) {
21632177
// 6) shared in enclosing context -> shared
2164-
useLastDeclSymbol();
2178+
makeSharedSymbol();
21652179
dsa = Symbol::Flag::OmpShared;
21662180
} else {
21672181
// 7) firstprivate
21682182
dsa = Symbol::Flag::OmpFirstPrivate;
2169-
declNewSymbol(*dsa)->set(Symbol::Flag::OmpImplicit);
2183+
makePrivateSymbol(*dsa)->set(Symbol::Flag::OmpImplicit);
21702184
}
21712185
}
21722186
prevDSA = dsa;
@@ -2570,20 +2584,59 @@ void ResolveOmpTopLevelParts(
25702584
});
25712585
}
25722586

2573-
void OmpAttributeVisitor::CheckDataCopyingClause(
2574-
const parser::Name &name, const Symbol &symbol, Symbol::Flag ompFlag) {
2575-
const auto *checkSymbol{&symbol};
2587+
static bool IsSymbolInCommonBlock(const Symbol &symbol) {
2588+
// TODO Improve the performance of this predicate function.
2589+
// Going through all symbols sequentially, in all common blocks, can be
2590+
// slow when there are many symbols. A possible optimization is to add
2591+
// an OmpInCommonBlock flag to Symbol, to make it possible to quickly
2592+
// test if a given symbol is in a common block.
2593+
for (const auto &cb : symbol.owner().commonBlocks()) {
2594+
if (IsCommonBlockContaining(cb.second.get(), symbol)) {
2595+
return true;
2596+
}
2597+
}
2598+
return false;
2599+
}
2600+
2601+
static bool IsSymbolThreadprivate(const Symbol &symbol) {
25762602
if (const auto *details{symbol.detailsIf<HostAssocDetails>()}) {
2577-
checkSymbol = &details->symbol();
2603+
return details->symbol().test(Symbol::Flag::OmpThreadprivate);
2604+
}
2605+
return symbol.test(Symbol::Flag::OmpThreadprivate);
2606+
}
2607+
2608+
static bool IsSymbolPrivate(const Symbol &symbol) {
2609+
if (symbol.test(Symbol::Flag::OmpPrivate) ||
2610+
symbol.test(Symbol::Flag::OmpFirstPrivate)) {
2611+
return true;
2612+
}
2613+
// A symbol that has not gone through constructs that may privatize the
2614+
// original symbol may be predetermined as private.
2615+
// (OMP 5.2 5.1.1 - Variables Referenced in a Construct)
2616+
if (symbol == symbol.GetUltimate()) {
2617+
switch (symbol.owner().kind()) {
2618+
case Scope::Kind::MainProgram:
2619+
case Scope::Kind::Subprogram:
2620+
case Scope::Kind::BlockConstruct:
2621+
return !symbol.attrs().test(Attr::SAVE) &&
2622+
!symbol.attrs().test(Attr::PARAMETER) && !IsAssumedShape(symbol) &&
2623+
!IsSymbolInCommonBlock(symbol);
2624+
default:
2625+
return false;
2626+
}
25782627
}
2628+
return false;
2629+
}
25792630

2631+
void OmpAttributeVisitor::CheckDataCopyingClause(
2632+
const parser::Name &name, const Symbol &symbol, Symbol::Flag ompFlag) {
25802633
if (ompFlag == Symbol::Flag::OmpCopyIn) {
25812634
// List of items/objects that can appear in a 'copyin' clause must be
25822635
// 'threadprivate'
2583-
if (!checkSymbol->test(Symbol::Flag::OmpThreadprivate)) {
2636+
if (!IsSymbolThreadprivate(symbol)) {
25842637
context_.Say(name.source,
25852638
"Non-THREADPRIVATE object '%s' in COPYIN clause"_err_en_US,
2586-
checkSymbol->name());
2639+
symbol.name());
25872640
}
25882641
} else if (ompFlag == Symbol::Flag::OmpCopyPrivate &&
25892642
GetContext().directive == llvm::omp::Directive::OMPD_single) {
@@ -2596,18 +2649,13 @@ void OmpAttributeVisitor::CheckDataCopyingClause(
25962649
"COPYPRIVATE variable '%s' may not appear on a PRIVATE or "
25972650
"FIRSTPRIVATE clause on a SINGLE construct"_err_en_US,
25982651
symbol.name());
2599-
} else {
2652+
} else if (!IsSymbolThreadprivate(symbol) && !IsSymbolPrivate(symbol)) {
26002653
// List of items/objects that can appear in a 'copyprivate' clause must be
26012654
// either 'private' or 'threadprivate' in enclosing context.
2602-
if (!checkSymbol->test(Symbol::Flag::OmpThreadprivate) &&
2603-
!(HasSymbolInEnclosingScope(symbol, currScope()) &&
2604-
(symbol.test(Symbol::Flag::OmpPrivate) ||
2605-
symbol.test(Symbol::Flag::OmpFirstPrivate)))) {
2606-
context_.Say(name.source,
2607-
"COPYPRIVATE variable '%s' is not PRIVATE or THREADPRIVATE in "
2608-
"outer context"_err_en_US,
2609-
symbol.name());
2610-
}
2655+
context_.Say(name.source,
2656+
"COPYPRIVATE variable '%s' is not PRIVATE or THREADPRIVATE in "
2657+
"outer context"_err_en_US,
2658+
symbol.name());
26112659
}
26122660
}
26132661
}
@@ -2677,12 +2725,6 @@ void OmpAttributeVisitor::CheckLabelContext(const parser::CharBlock source,
26772725
}
26782726
}
26792727

2680-
bool OmpAttributeVisitor::HasSymbolInEnclosingScope(
2681-
const Symbol &symbol, Scope &scope) {
2682-
const auto symbols{scope.parent().GetSymbols()};
2683-
return llvm::is_contained(symbols, symbol);
2684-
}
2685-
26862728
// Goes through the names in an OmpObjectList and checks if each name appears
26872729
// in the given allocate statement
26882730
void OmpAttributeVisitor::CheckAllNamesInAllocateStmt(

‎flang/test/Semantics/OpenMP/copyprivate04.f90‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
! RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp
2+
! OpenMP Version 5.2
3+
! 5.1.1 - Variables Referenced in a Construct
4+
! Copyprivate must accept variables that are predetermined as private.
5+
6+
module m1
7+
integer :: m
8+
end module
9+
10+
program omp_copyprivate
11+
use m1
12+
implicit none
13+
integer :: i
14+
integer, save :: j
15+
integer :: k
16+
common /c/ k
17+
real, parameter :: pi = 3.14
18+
integer :: a1(10)
19+
20+
! Local variables are private.
21+
!$omp single
22+
i = 123
23+
!$omp end single copyprivate(i)
24+
!$omp single
25+
!$omp end single copyprivate(a1)
26+
27+
! Variables with the SAVE attribute are not private.
28+
!$omp single
29+
!ERROR: COPYPRIVATE variable 'j' is not PRIVATE or THREADPRIVATE in outer context
30+
!$omp end single copyprivate(j)
31+
32+
! Common block variables are not private.
33+
!$omp single
34+
!ERROR: COPYPRIVATE variable 'k' is not PRIVATE or THREADPRIVATE in outer context
35+
!$omp end single copyprivate(/c/)
36+
!$omp single
37+
!ERROR: COPYPRIVATE variable 'k' is not PRIVATE or THREADPRIVATE in outer context
38+
!$omp end single copyprivate(k)
39+
40+
! Module variables are not private.
41+
!$omp single
42+
!ERROR: COPYPRIVATE variable 'm' is not PRIVATE or THREADPRIVATE in outer context
43+
!$omp end single copyprivate(m)
44+
45+
! Parallel can make a variable shared.
46+
!$omp parallel
47+
!$omp single
48+
i = 456
49+
!ERROR: COPYPRIVATE variable 'i' is not PRIVATE or THREADPRIVATE in outer context
50+
!$omp end single copyprivate(i)
51+
call sub(j, a1)
52+
!$omp end parallel
53+
54+
!$omp parallel shared(i)
55+
!$omp single
56+
i = 456
57+
!ERROR: COPYPRIVATE variable 'i' is not PRIVATE or THREADPRIVATE in outer context
58+
!$omp end single copyprivate(i)
59+
!$omp end parallel
60+
61+
!FIXME: an error should be emitted in this case.
62+
! copyprivate(i) should be considered as a reference to i and a new
63+
! symbol should be created in `parallel` scope, for this case to be
64+
! handled properly.
65+
!$omp parallel
66+
!$omp single
67+
!$omp end single copyprivate(i)
68+
!$omp end parallel
69+
70+
! Named constants are shared.
71+
!$omp single
72+
!ERROR: COPYPRIVATE variable 'pi' is not PRIVATE or THREADPRIVATE in outer context
73+
!$omp end single copyprivate(pi)
74+
75+
!$omp parallel do
76+
do i = 1, 10
77+
!$omp parallel
78+
!$omp single
79+
j = i
80+
!ERROR: COPYPRIVATE variable 'i' is not PRIVATE or THREADPRIVATE in outer context
81+
!$omp end single copyprivate(i)
82+
!$omp end parallel
83+
end do
84+
!$omp end parallel do
85+
86+
contains
87+
subroutine sub(s1, a)
88+
integer :: s1
89+
integer :: a(:)
90+
91+
! Dummy argument.
92+
!$omp single
93+
!$omp end single copyprivate(s1)
94+
95+
! Assumed shape arrays are shared.
96+
!$omp single
97+
!ERROR: COPYPRIVATE variable 'a' is not PRIVATE or THREADPRIVATE in outer context
98+
!$omp end single copyprivate(a)
99+
end subroutine
100+
101+
integer function fun(f1)
102+
integer :: f1
103+
104+
! Dummy argument.
105+
!$omp single
106+
!$omp end single copyprivate(f1)
107+
108+
! Function result is private.
109+
!$omp single
110+
!$omp end single copyprivate(fun)
111+
end function
112+
end program

‎flang/test/Semantics/OpenMP/do05-positivecase.f90‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,12 @@ program omp_do
2020
!$omp parallel default(shared)
2121
!$omp do
2222
!DEF: /omp_do/OtherConstruct2/OtherConstruct1/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
23-
!REF: /omp_do/n
23+
!DEF: /omp_do/OtherConstruct2/n HostAssoc INTEGER(4)
2424
do i=1,n
2525
!$omp parallel
2626
!$omp single
2727
!DEF: /work EXTERNAL (Subroutine) ProcEntity
28-
!REF: /omp_do/OtherConstruct2/OtherConstruct1/i
28+
!DEF: /omp_do/OtherConstruct2/OtherConstruct1/OtherConstruct1/i HostAssoc INTEGER(4)
2929
call work(i, 1)
3030
!$omp end single
3131
!$omp end parallel

‎flang/test/Semantics/OpenMP/do20.f90‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ subroutine shared_iv
1010

1111
!$omp parallel shared(i)
1212
!$omp single
13-
!REF: /shared_iv/i
13+
!DEF: /shared_iv/OtherConstruct1/i HostAssoc INTEGER(4)
1414
do i = 0, 1
1515
end do
1616
!$omp end single

0 commit comments

Comments
 (0)