Skip to content

Commit

Permalink
[flang][OpenMP] Reland Fix copyprivate semantic checks (llvm#95799)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@luporl
luporl committed Jul 29, 2024
1 parent 68343d7 commit 3922be2
Show file tree
Hide file tree
Showing 15 changed files with 250 additions and 95 deletions.
1 change: 1 addition & 0 deletions flang/include/flang/Semantics/tools.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ bool IsIntrinsicConcat(
bool IsGenericDefinedOp(const Symbol &);
bool IsDefinedOperator(SourceName);
std::string MakeOpName(SourceName);
bool IsCommonBlockContaining(const Symbol &, const Symbol &);

// Returns true if maybeAncestor exists and is a proper ancestor of a
// descendent scope (or symbol owner). Will be false, unlike Scope::Contains(),
Expand Down
138 changes: 90 additions & 48 deletions flang/lib/Semantics/resolve-directives.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "flang/Parser/parse-tree.h"
#include "flang/Parser/tools.h"
#include "flang/Semantics/expression.h"
#include "flang/Semantics/tools.h"
#include <list>
#include <map>
#include <sstream>
Expand Down Expand Up @@ -729,7 +730,6 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
void CheckNameInAllocateStmt(const parser::CharBlock &source,
const parser::Name &ompObject, const parser::AllocateStmt &allocate);

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

void AddOmpRequiresToScope(Scope &, WithOmpDeclarative::RequiresFlags,
Expand Down Expand Up @@ -2085,16 +2085,22 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
}
}

// When handling each implicit rule, either a new private symbol is
// declared or the last declared symbol is used.
// In the latter case, it's necessary to insert a new symbol in the scope
// being processed, associated with the last declared symbol.
// This captures the fact that, although we are using the last declared
// symbol, its DSA could be different in this scope.
// Also, because of how symbols are collected in lowering, not inserting
// a new symbol in this scope could lead to the conclusion that the
// symbol was declared in this construct, which would result in wrong
// privatization code being generated.
// When handling each implicit rule for a given symbol, one of the
// following 3 actions may be taken:
// 1. Declare a new private symbol.
// 2. Create a new association symbol with no flags, that will represent
// a shared symbol in the current scope. Note that symbols without
// any private flags are considered as shared.
// 3. Use the last declared private symbol, by inserting a new symbol
// in the scope being processed, associated with it.
// If no private symbol was declared previously, then no association
// is needed and the symbol from the enclosing scope will be
// inherited by the current one.
//
// Because of how symbols are collected in lowering, not inserting a new
// symbol in the last case could lead to the conclusion that a symbol
// from an enclosing construct was declared in the current construct,
// which would result in wrong privatization code being generated.
// Consider the following example:
//
// !$omp parallel default(private) ! p1
Expand All @@ -2107,48 +2113,56 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
// (p2), it would use the x symbol definition from the enclosing scope.
// Then, when p2's default symbols were collected in lowering, the x
// symbol from the outer parallel construct (p1) would be collected, as
// it would have the private flag set (note that symbols that don't have
// any private flag are considered as shared).
// it would have the private flag set.
// This would make x appear to be defined in p2, causing it to be
// privatized in p2 and its privatization in p1 to be skipped.
auto declNewSymbol = [&](Symbol::Flag flag) {
auto makePrivateSymbol = [&](Symbol::Flag flag) {
Symbol *hostSymbol =
lastDeclSymbol ? lastDeclSymbol : &symbol->GetUltimate();
lastDeclSymbol = DeclarePrivateAccessEntity(
*hostSymbol, flag, context_.FindScope(dirContext.directiveSource));
return lastDeclSymbol;
};
auto makeSharedSymbol = [&]() {
Symbol *hostSymbol =
lastDeclSymbol ? lastDeclSymbol : &symbol->GetUltimate();
MakeAssocSymbol(symbol->name(), *hostSymbol,
context_.FindScope(dirContext.directiveSource));
};
auto useLastDeclSymbol = [&]() {
if (lastDeclSymbol)
MakeAssocSymbol(symbol->name(), *lastDeclSymbol,
context_.FindScope(dirContext.directiveSource));
};

bool taskGenDir = llvm::omp::taskGeneratingSet.test(dirContext.directive);
bool targetDir = llvm::omp::allTargetSet.test(dirContext.directive);
bool parallelDir = llvm::omp::allParallelSet.test(dirContext.directive);
bool teamsDir = llvm::omp::allTeamsSet.test(dirContext.directive);

if (dsa.has_value()) {
useLastDeclSymbol();
if (dsa.value() == Symbol::Flag::OmpShared &&
(parallelDir || taskGenDir || teamsDir))
makeSharedSymbol();
// Private symbols will have been declared already.
prevDSA = dsa;
continue;
}

bool taskGenDir = llvm::omp::taskGeneratingSet.test(dirContext.directive);
bool targetDir = llvm::omp::allTargetSet.test(dirContext.directive);
bool parallelDir = llvm::omp::allParallelSet.test(dirContext.directive);

if (dirContext.defaultDSA == Symbol::Flag::OmpPrivate ||
dirContext.defaultDSA == Symbol::Flag::OmpFirstPrivate ||
dirContext.defaultDSA == Symbol::Flag::OmpShared) {
// 1) default
// Allowed only with parallel, teams and task generating constructs.
assert(parallelDir || taskGenDir ||
llvm::omp::allTeamsSet.test(dirContext.directive));
assert(parallelDir || taskGenDir || teamsDir);
if (dirContext.defaultDSA != Symbol::Flag::OmpShared)
declNewSymbol(dirContext.defaultDSA);
makePrivateSymbol(dirContext.defaultDSA);
else
useLastDeclSymbol();
makeSharedSymbol();
dsa = dirContext.defaultDSA;
} else if (parallelDir) {
// 2) parallel -> shared
useLastDeclSymbol();
makeSharedSymbol();
dsa = Symbol::Flag::OmpShared;
} else if (!taskGenDir && !targetDir) {
// 3) enclosing context
Expand All @@ -2161,12 +2175,12 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
// TODO 5) dummy arg in orphaned taskgen construct -> firstprivate
if (prevDSA == Symbol::Flag::OmpShared) {
// 6) shared in enclosing context -> shared
useLastDeclSymbol();
makeSharedSymbol();
dsa = Symbol::Flag::OmpShared;
} else {
// 7) firstprivate
dsa = Symbol::Flag::OmpFirstPrivate;
declNewSymbol(*dsa)->set(Symbol::Flag::OmpImplicit);
makePrivateSymbol(*dsa)->set(Symbol::Flag::OmpImplicit);
}
}
prevDSA = dsa;
Expand Down Expand Up @@ -2570,20 +2584,59 @@ void ResolveOmpTopLevelParts(
});
}

void OmpAttributeVisitor::CheckDataCopyingClause(
const parser::Name &name, const Symbol &symbol, Symbol::Flag ompFlag) {
const auto *checkSymbol{&symbol};
static bool IsSymbolInCommonBlock(const Symbol &symbol) {
// TODO Improve the performance of this predicate function.
// Going through all symbols sequentially, in all common blocks, can be
// slow when there are many symbols. A possible optimization is to add
// an OmpInCommonBlock flag to Symbol, to make it possible to quickly
// test if a given symbol is in a common block.
for (const auto &cb : symbol.owner().commonBlocks()) {
if (IsCommonBlockContaining(cb.second.get(), symbol)) {
return true;
}
}
return false;
}

static bool IsSymbolThreadprivate(const Symbol &symbol) {
if (const auto *details{symbol.detailsIf<HostAssocDetails>()}) {
checkSymbol = &details->symbol();
return details->symbol().test(Symbol::Flag::OmpThreadprivate);
}
return symbol.test(Symbol::Flag::OmpThreadprivate);
}

static bool IsSymbolPrivate(const Symbol &symbol) {
if (symbol.test(Symbol::Flag::OmpPrivate) ||
symbol.test(Symbol::Flag::OmpFirstPrivate)) {
return true;
}
// A symbol that has not gone through constructs that may privatize the
// original symbol may be predetermined as private.
// (OMP 5.2 5.1.1 - Variables Referenced in a Construct)
if (symbol == symbol.GetUltimate()) {
switch (symbol.owner().kind()) {
case Scope::Kind::MainProgram:
case Scope::Kind::Subprogram:
case Scope::Kind::BlockConstruct:
return !symbol.attrs().test(Attr::SAVE) &&
!symbol.attrs().test(Attr::PARAMETER) && !IsAssumedShape(symbol) &&
!IsSymbolInCommonBlock(symbol);
default:
return false;
}
}
return false;
}

void OmpAttributeVisitor::CheckDataCopyingClause(
const parser::Name &name, const Symbol &symbol, Symbol::Flag ompFlag) {
if (ompFlag == Symbol::Flag::OmpCopyIn) {
// List of items/objects that can appear in a 'copyin' clause must be
// 'threadprivate'
if (!checkSymbol->test(Symbol::Flag::OmpThreadprivate)) {
if (!IsSymbolThreadprivate(symbol)) {
context_.Say(name.source,
"Non-THREADPRIVATE object '%s' in COPYIN clause"_err_en_US,
checkSymbol->name());
symbol.name());
}
} else if (ompFlag == Symbol::Flag::OmpCopyPrivate &&
GetContext().directive == llvm::omp::Directive::OMPD_single) {
Expand All @@ -2596,18 +2649,13 @@ void OmpAttributeVisitor::CheckDataCopyingClause(
"COPYPRIVATE variable '%s' may not appear on a PRIVATE or "
"FIRSTPRIVATE clause on a SINGLE construct"_err_en_US,
symbol.name());
} else {
} else if (!IsSymbolThreadprivate(symbol) && !IsSymbolPrivate(symbol)) {
// List of items/objects that can appear in a 'copyprivate' clause must be
// either 'private' or 'threadprivate' in enclosing context.
if (!checkSymbol->test(Symbol::Flag::OmpThreadprivate) &&
!(HasSymbolInEnclosingScope(symbol, currScope()) &&
(symbol.test(Symbol::Flag::OmpPrivate) ||
symbol.test(Symbol::Flag::OmpFirstPrivate)))) {
context_.Say(name.source,
"COPYPRIVATE variable '%s' is not PRIVATE or THREADPRIVATE in "
"outer context"_err_en_US,
symbol.name());
}
context_.Say(name.source,
"COPYPRIVATE variable '%s' is not PRIVATE or THREADPRIVATE in "
"outer context"_err_en_US,
symbol.name());
}
}
}
Expand Down Expand Up @@ -2677,12 +2725,6 @@ void OmpAttributeVisitor::CheckLabelContext(const parser::CharBlock source,
}
}

bool OmpAttributeVisitor::HasSymbolInEnclosingScope(
const Symbol &symbol, Scope &scope) {
const auto symbols{scope.parent().GetSymbols()};
return llvm::is_contained(symbols, symbol);
}

// Goes through the names in an OmpObjectList and checks if each name appears
// in the given allocate statement
void OmpAttributeVisitor::CheckAllNamesInAllocateStmt(
Expand Down
112 changes: 112 additions & 0 deletions flang/test/Semantics/OpenMP/copyprivate04.f90
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
! RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp
! OpenMP Version 5.2
! 5.1.1 - Variables Referenced in a Construct
! Copyprivate must accept variables that are predetermined as private.

module m1
integer :: m
end module

program omp_copyprivate
use m1
implicit none
integer :: i
integer, save :: j
integer :: k
common /c/ k
real, parameter :: pi = 3.14
integer :: a1(10)

! Local variables are private.
!$omp single
i = 123
!$omp end single copyprivate(i)
!$omp single
!$omp end single copyprivate(a1)

! Variables with the SAVE attribute are not private.
!$omp single
!ERROR: COPYPRIVATE variable 'j' is not PRIVATE or THREADPRIVATE in outer context
!$omp end single copyprivate(j)

! Common block variables are not private.
!$omp single
!ERROR: COPYPRIVATE variable 'k' is not PRIVATE or THREADPRIVATE in outer context
!$omp end single copyprivate(/c/)
!$omp single
!ERROR: COPYPRIVATE variable 'k' is not PRIVATE or THREADPRIVATE in outer context
!$omp end single copyprivate(k)

! Module variables are not private.
!$omp single
!ERROR: COPYPRIVATE variable 'm' is not PRIVATE or THREADPRIVATE in outer context
!$omp end single copyprivate(m)

! Parallel can make a variable shared.
!$omp parallel
!$omp single
i = 456
!ERROR: COPYPRIVATE variable 'i' is not PRIVATE or THREADPRIVATE in outer context
!$omp end single copyprivate(i)
call sub(j, a1)
!$omp end parallel

!$omp parallel shared(i)
!$omp single
i = 456
!ERROR: COPYPRIVATE variable 'i' is not PRIVATE or THREADPRIVATE in outer context
!$omp end single copyprivate(i)
!$omp end parallel

!FIXME: an error should be emitted in this case.
! copyprivate(i) should be considered as a reference to i and a new
! symbol should be created in `parallel` scope, for this case to be
! handled properly.
!$omp parallel
!$omp single
!$omp end single copyprivate(i)
!$omp end parallel

! Named constants are shared.
!$omp single
!ERROR: COPYPRIVATE variable 'pi' is not PRIVATE or THREADPRIVATE in outer context
!$omp end single copyprivate(pi)

!$omp parallel do
do i = 1, 10
!$omp parallel
!$omp single
j = i
!ERROR: COPYPRIVATE variable 'i' is not PRIVATE or THREADPRIVATE in outer context
!$omp end single copyprivate(i)
!$omp end parallel
end do
!$omp end parallel do

contains
subroutine sub(s1, a)
integer :: s1
integer :: a(:)

! Dummy argument.
!$omp single
!$omp end single copyprivate(s1)

! Assumed shape arrays are shared.
!$omp single
!ERROR: COPYPRIVATE variable 'a' is not PRIVATE or THREADPRIVATE in outer context
!$omp end single copyprivate(a)
end subroutine

integer function fun(f1)
integer :: f1

! Dummy argument.
!$omp single
!$omp end single copyprivate(f1)

! Function result is private.
!$omp single
!$omp end single copyprivate(fun)
end function
end program
4 changes: 2 additions & 2 deletions flang/test/Semantics/OpenMP/do05-positivecase.f90
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ program omp_do
!$omp parallel default(shared)
!$omp do
!DEF: /omp_do/OtherConstruct2/OtherConstruct1/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
!REF: /omp_do/n
!DEF: /omp_do/OtherConstruct2/n HostAssoc INTEGER(4)
do i=1,n
!$omp parallel
!$omp single
!DEF: /work EXTERNAL (Subroutine) ProcEntity
!REF: /omp_do/OtherConstruct2/OtherConstruct1/i
!DEF: /omp_do/OtherConstruct2/OtherConstruct1/OtherConstruct1/i HostAssoc INTEGER(4)
call work(i, 1)
!$omp end single
!$omp end parallel
Expand Down
2 changes: 1 addition & 1 deletion flang/test/Semantics/OpenMP/do20.f90
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ subroutine shared_iv

!$omp parallel shared(i)
!$omp single
!REF: /shared_iv/i
!DEF: /shared_iv/OtherConstruct1/i HostAssoc INTEGER(4)
do i = 0, 1
end do
!$omp end single
Expand Down
Loading

0 comments on commit 3922be2

Please sign in to comment.