llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-flang-openmp Author: None (llvmbot) <details> <summary>Changes</summary> Backport eb9bf188918bf5c88151d7735d925a3912a5b596 Requested by: @<!-- -->luporl --- Patch is 29.74 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/100352.diff 15 Files Affected: - (modified) flang/include/flang/Semantics/tools.h (+1) - (modified) flang/lib/Semantics/resolve-directives.cpp (+90-48) - (added) flang/test/Semantics/OpenMP/copyprivate04.f90 (+112) - (modified) flang/test/Semantics/OpenMP/do05-positivecase.f90 (+2-2) - (modified) flang/test/Semantics/OpenMP/do20.f90 (+1-1) - (modified) flang/test/Semantics/OpenMP/implicit-dsa.f90 (+7-7) - (modified) flang/test/Semantics/OpenMP/reduction08.f90 (+5-5) - (modified) flang/test/Semantics/OpenMP/reduction09.f90 (+4-4) - (modified) flang/test/Semantics/OpenMP/symbol01.f90 (+2-2) - (modified) flang/test/Semantics/OpenMP/symbol02.f90 (+2-2) - (modified) flang/test/Semantics/OpenMP/symbol03.f90 (+2-2) - (modified) flang/test/Semantics/OpenMP/symbol05.f90 (+2-2) - (modified) flang/test/Semantics/OpenMP/symbol07.f90 (+1-1) - (modified) flang/test/Semantics/OpenMP/symbol08.f90 (+18-18) - (modified) flang/test/Semantics/OpenMP/symbol09.f90 (+1-1) ``````````diff diff --git a/flang/include/flang/Semantics/tools.h b/flang/include/flang/Semantics/tools.h index 0fcba3131fad1..f21ef28618142 100644 --- a/flang/include/flang/Semantics/tools.h +++ b/flang/include/flang/Semantics/tools.h @@ -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(), diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp index 16658d50c151c..ddb928409ee74 100644 --- a/flang/lib/Semantics/resolve-directives.cpp +++ b/flang/lib/Semantics/resolve-directives.cpp @@ -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> @@ -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, @@ -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 @@ -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 @@ -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; @@ -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) { @@ -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()); } } } @@ -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( diff --git a/flang/test/Semantics/OpenMP/copyprivate04.f90 b/flang/test/Semantics/OpenMP/copyprivate04.f90 new file mode 100644 index 0000000000000..291cf1103fb27 --- /dev/null +++ b/flang/test/Semantics/OpenMP/copyprivate04.f90 @@ -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 diff --git a/flang/test/Semantics/OpenMP/do05-positivecase.f90 b/flang/test/Semantics/OpenMP/do05-positivecase.f90 index 4e02235f58a1a..3b512a5b4f25e 100644 --- a/flang/test/Semantics/OpenMP/do05-positivecase.f90 +++ b/flang/test/Semantics/OpenMP/do05-positivecase.f90 @@ -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 diff --git a/flang/test/Semantics/OpenMP/do20.f90 b/flang/test/Semantics/OpenMP/do20.f90 index 915d01e69edd7..0cafae76b86b0 100644 --- a/flang/test/Semantics/OpenMP/do20.f90 +++ b/flang/test/Semantics/OpenMP/do20.f90 @@ -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 diff --git a/flang/test/Semantics/OpenMP/implicit-dsa.f90 b/flang/test/Semantics/OpenMP/implicit-dsa.f90 index 92d2421d06f97..2abe3a0e16d62 100644 --- a/flang/test/Semantics/OpenMP/implicit-dsa.f90 +++ b/flang/test/Semantics/OpenMP/implicit-dsa.f90 @@ -15,14 +15,14 @@ subroutine implicit_dsa_test1 !$omp task private(y) shared(z) !DEF: /implicit_dsa_test1/OtherConstruct1/x (OmpFirstPrivate, OmpImplicit) HostAssoc INTEGER(4) !DEF: /implicit_dsa_test1/OtherConstruct1/y (OmpPrivate) HostAssoc INTEGER(4) - !REF: /implicit_dsa_test1/z + !DEF: /implicit_dsa_test1/OtherConstruct1/z HostAssoc INTEGER(4) x = y + z !$omp end task !$omp task default(shared) - !REF: /implicit_dsa_test1/x - !REF: /implicit_dsa_test1/y - !REF: /implicit_dsa_test1/z + !DEF: /implicit_dsa_test1/OtherConstruct2/x HostAssoc INTEGER(4) + !DEF: /implicit_dsa_test1/OtherConstruct2/y HostAssoc INTEGER(4) + !DEF: /implicit_dsa_test1/OtherConstruct2/z HostAssoc INTEGER(4) x = y + z !$omp end task @@ -61,16 +61,16 @@ subroutine implicit_dsa_test3 !$omp parallel !$omp task - !REF: /implicit_dsa_test3/x + !DEF: /implicit_dsa_test3/OtherConstruct1/OtherConstruct1/x HostAssoc INTEGER(4) x = 1 - !REF: /implicit_dsa_test3/y + !DEF: /implicit_dsa_test3/OtherConstruct1/OtherConstruct1/y HostAssoc INTEGER(4) y = 1 !$omp end task !$omp task firstprivate(x) !DEF: /implicit_dsa_test3/OtherConstruct1/OtherConstruct2/x (OmpFirstPrivate) HostAssoc INTEGER(4) x = 1 - !REF: /implicit_dsa_test3/z + !DEF: /implicit_dsa_test3/OtherConstruct1/OtherConstruct2/z HostAssoc INTEGER(4) z = 1 !$omp end task !$omp end parallel diff --git a/flang/test/Semantics/OpenMP/reduction08.f90 b/flang/test/Semantics/OpenMP/reduction08.f90 index 99163327cdafa..9442fbd4d5978 100644 --- a/flang/test/Semantics/OpenMP/reduction08.f90 +++ b/flang/test/Semantics/OpenMP/reduction08.f90 @@ -15,7 +15,7 @@ program omp_reduction do i=1,10 !DEF: /omp_reduction/OtherConstruct1/k (OmpReduction) HostAssoc INTEGER(4) !DEF: /omp_reduction/max ELEMENTAL, INTRINSIC, PURE (Function) ProcEntity - !REF: /omp_reduction/m + !DEF: /omp_reduction/OtherConstruct1/m HostAssoc INTEGER(4) k = max(k, m) end do !$omp end parallel do @@ -25,7 +25,7 @@ program omp_reduction do i=1,10 !DEF: /omp_reduction/OtherConstruct2/k (OmpReduction) HostAssoc INTEGER(4) !DEF: /omp_reduction/min ELEMENTAL, INTRINSIC, PURE (Function) ProcEntity - !REF: /omp_reduction/m + !DEF: /omp_reduction/OtherConstruct2/m HostAssoc INTEGER(4) k = min(k, m) end do !$omp end parallel do @@ -35,7 +35,7 @@ program omp_reduction do i=1,10 !DEF: /omp_reduction/OtherConstruct3/k (OmpReduction) HostAssoc INTEGER(4) !DEF: /omp_reduction/iand ELEMENTAL, INTRINSIC, PURE (Function) ProcEntity - !REF: /omp_reduction/m + !DEF: /omp_reduction/OtherConstruct3/m HostAssoc INTEGER(4) k = iand(k, m) end do !$omp end parallel do @@ -45,7 +45,7 @@ program omp_reduction do i=1,10 !DEF: /omp_reduction/OtherConstruct4/k (OmpReduction) HostAssoc INTEGER(4) !DEF: /omp_reduction/ior ELEMENTAL, INTRINSIC, PURE (Function) ProcEntity - !REF: /omp_reduction/m + !DEF: /omp_reduction/OtherConstruct4/m HostAssoc INTEGER(4) k = ior(k, m) end do !$omp end parallel do @@ -55,7 +55,7 @@ program omp_reduction do i=1,10 !DEF: /omp_reduction/OtherConstruct5/k (OmpReduction) HostAssoc INTEGER(4) !DEF: /omp_reduction/ieor ELEMENTAL, INTRINSIC, PURE (Function) ProcEntity - !REF: /omp_reduction/m + !DEF: /omp_reduction/OtherConstruct5/m HostAssoc INTEGER(4) k = ieor(k,m) end do !$omp end parallel do diff --git a/flang/test/Semantics/OpenMP/reduction09.f90 b/flang/test/Semantics/OpenMP/reduction09.f90 index dbc8d1b060e65..1af2fc4fd9691 100644 --- a/flang/test/Semantics/OpenMP/reduction09.f90 +++ b/flang/test/Semantics/OpenMP/reduction09.f90 @@ -26,7 +26,7 @@ program omp_reduction !$omp parallel do reduction(+:a(10)) !DEF: /omp_reduction/OtherConstruct2/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4) do i=1,10 - !REF: /om... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/100352 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
