From 78437ade2a8e247681b5cf40c560d19dc17b3bca Mon Sep 17 00:00:00 2001 From: Peter Klausler Date: Mon, 20 Oct 2025 13:08:25 -0700 Subject: [PATCH] [flang] Address OpenACC name resolution gaps Some OpenACC parsers aren't filling in the "source" data members of parse tree nodes, or not doing so correctly; and some of those nodes are not adding their source data members to the source ranges of the current scope when being visited in name resolution, which causes SemanticsContext::FindScope() to misidentify the current scope in directive resolution when creating contexts. Further, the name resolution for a "use_device" clause isn't walking its subtrees, so some parser::Name nodes are not being resolved to Symbols. Fix these problems, and clean up resolve-directives.cpp a bit, since most Name nodes don't need to have their symbol table pointers updated now. --- flang/include/flang/Semantics/semantics.h | 1 + flang/lib/Parser/openacc-parsers.cpp | 71 ++++++++-------- flang/lib/Semantics/resolve-directives.cpp | 82 +++++++++---------- flang/lib/Semantics/resolve-names.cpp | 38 ++++++--- flang/lib/Semantics/semantics.cpp | 9 ++ .../OpenACC/openacc-type-categories-class.f90 | 2 +- flang/test/Semantics/OpenACC/bug1583.f90 | 23 ++++++ 7 files changed, 134 insertions(+), 92 deletions(-) create mode 100644 flang/test/Semantics/OpenACC/bug1583.f90 diff --git a/flang/include/flang/Semantics/semantics.h b/flang/include/flang/Semantics/semantics.h index f7910ad38a19d..c03d0a02d7faf 100644 --- a/flang/include/flang/Semantics/semantics.h +++ b/flang/include/flang/Semantics/semantics.h @@ -262,6 +262,7 @@ class SemanticsContext { const Scope &FindScope(parser::CharBlock) const; Scope &FindScope(parser::CharBlock); void UpdateScopeIndex(Scope &, parser::CharBlock); + void DumpScopeIndex(llvm::raw_ostream &) const; bool IsInModuleFile(parser::CharBlock) const; diff --git a/flang/lib/Parser/openacc-parsers.cpp b/flang/lib/Parser/openacc-parsers.cpp index ad035e6ade321..0dec56521f750 100644 --- a/flang/lib/Parser/openacc-parsers.cpp +++ b/flang/lib/Parser/openacc-parsers.cpp @@ -75,21 +75,21 @@ TYPE_PARSER( // tile size is one of: // * (represented as an empty std::optional) // constant-int-expr -TYPE_PARSER(construct(scalarIntConstantExpr) || +TYPE_PARSER(sourced(construct(scalarIntConstantExpr) || construct( - "*" >> construct>())) + "*" >> construct>()))) TYPE_PARSER(construct(nonemptyList(Parser{}))) // 2.9 (1979-1982) gang-arg is one of : // [num:]int-expr // dim:int-expr // static:size-expr -TYPE_PARSER(construct(construct( - "STATIC: " >> Parser{})) || +TYPE_PARSER(sourced(construct(construct( + "STATIC: " >> Parser{})) || construct( construct("DIM: " >> scalarIntExpr)) || construct( - construct(maybe("NUM: "_tok) >> scalarIntExpr))) + construct(maybe("NUM: "_tok) >> scalarIntExpr)))) // 2.9 gang-arg-list TYPE_PARSER( @@ -101,7 +101,7 @@ TYPE_PARSER(construct( // 2.5.15 Reduction, F'2023 R1131, and CUF reduction-op // Operator for reduction -TYPE_PARSER(sourced(construct( +TYPE_PARSER(construct( first("+" >> pure(ReductionOperator::Operator::Plus), "*" >> pure(ReductionOperator::Operator::Multiply), "MAX" >> pure(ReductionOperator::Operator::Max), @@ -112,32 +112,32 @@ TYPE_PARSER(sourced(construct( ".AND." >> pure(ReductionOperator::Operator::And), ".OR." >> pure(ReductionOperator::Operator::Or), ".EQV." >> pure(ReductionOperator::Operator::Eqv), - ".NEQV." >> pure(ReductionOperator::Operator::Neqv))))) + ".NEQV." >> pure(ReductionOperator::Operator::Neqv)))) // 2.15.1 Bind clause -TYPE_PARSER(sourced(construct(name)) || - sourced(construct(scalarDefaultCharExpr))) +TYPE_PARSER(sourced(construct(name) || + construct(scalarDefaultCharExpr))) // 2.5.16 Default clause -TYPE_PARSER(construct( +TYPE_PARSER(sourced(construct( first("NONE" >> pure(llvm::acc::DefaultValue::ACC_Default_none), - "PRESENT" >> pure(llvm::acc::DefaultValue::ACC_Default_present)))) + "PRESENT" >> pure(llvm::acc::DefaultValue::ACC_Default_present))))) // SELF clause is either a simple optional condition for compute construct // or a synonym of the HOST clause for the update directive 2.14.4 holding // an object list. -TYPE_PARSER( +TYPE_PARSER(sourced( construct(Parser{}) / lookAhead(")"_tok) || - construct(scalarLogicalExpr / lookAhead(")"_tok)) || + construct(scalarLogicalExpr) / lookAhead(")"_tok) || construct( recovery(fail>( "logical expression or object list expected"_err_en_US), - SkipTo<')'>{} >> pure>()))) + SkipTo<')'>{} >> pure>())))) // Modifier for copyin, copyout, cache and create -TYPE_PARSER(construct( +TYPE_PARSER(sourced(construct( first("ZERO:" >> pure(AccDataModifier::Modifier::Zero), - "READONLY:" >> pure(AccDataModifier::Modifier::ReadOnly)))) + "READONLY:" >> pure(AccDataModifier::Modifier::ReadOnly))))) // Combined directives TYPE_PARSER(sourced(construct( @@ -166,14 +166,13 @@ TYPE_PARSER(sourced(construct( TYPE_PARSER(sourced(construct( first("LOOP" >> pure(llvm::acc::Directive::ACCD_loop))))) -TYPE_PARSER(construct( - sourced(Parser{}), Parser{})) +TYPE_PARSER(sourced(construct( + Parser{}, Parser{}))) TYPE_PARSER(construct("END LOOP"_tok)) TYPE_PARSER(construct( - sourced(Parser{} / endAccLine), - maybe(Parser{}), + Parser{} / endAccLine, maybe(Parser{}), maybe(startAccLine >> Parser{} / endAccLine))) // 2.15.1 Routine directive @@ -186,8 +185,8 @@ TYPE_PARSER(sourced( parenthesized(Parser{})))) // 2.11 Combined constructs -TYPE_PARSER(construct( - sourced(Parser{}), Parser{})) +TYPE_PARSER(sourced(construct( + Parser{}, Parser{}))) // 2.12 Atomic constructs TYPE_PARSER(construct(startAccLine >> "END ATOMIC"_tok)) @@ -213,10 +212,10 @@ TYPE_PARSER("ATOMIC" >> statement(assignmentStmt), Parser{} / endAccLine)) TYPE_PARSER( - sourced(construct(Parser{})) || - sourced(construct(Parser{})) || - sourced(construct(Parser{})) || - sourced(construct(Parser{}))) + sourced(construct(Parser{}) || + construct(Parser{}) || + construct(Parser{}) || + construct(Parser{}))) // 2.13 Declare constructs TYPE_PARSER(sourced(construct( @@ -250,18 +249,18 @@ TYPE_PARSER(construct( pure(llvm::acc::Directive::ACCD_data)))))) // Standalone constructs -TYPE_PARSER(construct( - sourced(Parser{}), Parser{})) +TYPE_PARSER(sourced(construct( + Parser{}, Parser{}))) // Standalone declarative constructs -TYPE_PARSER(construct( - sourced(Parser{}), Parser{})) +TYPE_PARSER(sourced(construct( + Parser{}, Parser{}))) TYPE_PARSER(startAccLine >> withMessage("expected OpenACC directive"_err_en_US, - first(sourced(construct( - Parser{})), - sourced(construct( + sourced(first(construct( + Parser{}), + construct( Parser{}))))) TYPE_PARSER(sourced(construct( @@ -293,9 +292,9 @@ TYPE_PARSER(startAccLine >> "SERIAL"_tok >> maybe("LOOP"_tok) >> pure(llvm::acc::Directive::ACCD_serial_loop)))))) -TYPE_PARSER(construct( - sourced(Parser{} / endAccLine), +TYPE_PARSER(sourced(construct( + Parser{} / endAccLine, maybe(Parser{}), - maybe(Parser{} / endAccLine))) + maybe(Parser{} / endAccLine)))) } // namespace Fortran::parser diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp index 3bb586c51c58f..6c913fe67f10a 100644 --- a/flang/lib/Semantics/resolve-directives.cpp +++ b/flang/lib/Semantics/resolve-directives.cpp @@ -31,15 +31,17 @@ #include #include +namespace Fortran::semantics { + template -static Fortran::semantics::Scope *GetScope( - Fortran::semantics::SemanticsContext &context, const T &x) { - std::optional source{GetLastSource(x)}; - return source ? &context.FindScope(*source) : nullptr; +static Scope *GetScope(SemanticsContext &context, const T &x) { + if (auto source{GetLastSource(x)}) { + return &context.FindScope(*source); + } else { + return nullptr; + } } -namespace Fortran::semantics { - template class DirectiveAttributeVisitor { public: explicit DirectiveAttributeVisitor(SemanticsContext &context) @@ -361,7 +363,7 @@ class AccAttributeVisitor : DirectiveAttributeVisitor { void ResolveAccObject(const parser::AccObject &, Symbol::Flag); Symbol *ResolveAcc(const parser::Name &, Symbol::Flag, Scope &); Symbol *ResolveAcc(Symbol &, Symbol::Flag, Scope &); - Symbol *ResolveName(const parser::Name &, bool parentScope = false); + Symbol *ResolveName(const parser::Name &); Symbol *ResolveFctName(const parser::Name &); Symbol *ResolveAccCommonBlockName(const parser::Name *); Symbol *DeclareOrMarkOtherAccessEntity(const parser::Name &, Symbol::Flag); @@ -1257,31 +1259,22 @@ bool AccAttributeVisitor::Pre(const parser::OpenACCStandaloneConstruct &x) { return true; } -Symbol *AccAttributeVisitor::ResolveName( - const parser::Name &name, bool parentScope) { - Symbol *prev{currScope().FindSymbol(name.source)}; - // Check in parent scope if asked for. - if (!prev && parentScope) { - prev = currScope().parent().FindSymbol(name.source); - } - if (prev != name.symbol) { - name.symbol = prev; - } - return prev; +Symbol *AccAttributeVisitor::ResolveName(const parser::Name &name) { + return name.symbol; } Symbol *AccAttributeVisitor::ResolveFctName(const parser::Name &name) { Symbol *prev{currScope().FindSymbol(name.source)}; - if (!prev || (prev && prev->IsFuncResult())) { + if (prev && prev->IsFuncResult()) { prev = currScope().parent().FindSymbol(name.source); - if (!prev) { - prev = &context_.globalScope().MakeSymbol( - name.source, Attrs{}, ProcEntityDetails{}); - } } - if (prev != name.symbol) { - name.symbol = prev; + if (!prev) { + prev = &*context_.globalScope() + .try_emplace(name.source, ProcEntityDetails{}) + .first->second; } + CHECK(!name.symbol || name.symbol == prev); + name.symbol = prev; return prev; } @@ -1388,9 +1381,8 @@ bool AccAttributeVisitor::Pre(const parser::OpenACCRoutineConstruct &x) { } else { PushContext(verbatim.source, llvm::acc::Directive::ACCD_routine); } - const auto &optName{std::get>(x.t)}; - if (optName) { - if (Symbol *sym = ResolveFctName(*optName)) { + if (const auto &optName{std::get>(x.t)}) { + if (Symbol * sym{ResolveFctName(*optName)}) { Symbol &ultimate{sym->GetUltimate()}; AddRoutineInfoToSymbol(ultimate, x); } else { @@ -1425,7 +1417,7 @@ bool AccAttributeVisitor::Pre(const parser::OpenACCCombinedConstruct &x) { case llvm::acc::Directive::ACCD_kernels_loop: case llvm::acc::Directive::ACCD_parallel_loop: case llvm::acc::Directive::ACCD_serial_loop: - PushContext(combinedDir.source, combinedDir.v); + PushContext(x.source, combinedDir.v); break; default: break; @@ -1706,26 +1698,27 @@ void AccAttributeVisitor::Post(const parser::AccDefaultClause &x) { } } -// For OpenACC constructs, check all the data-refs within the constructs -// and adjust the symbol for each Name if necessary void AccAttributeVisitor::Post(const parser::Name &name) { - auto *symbol{name.symbol}; - if (symbol && WithinConstruct()) { - symbol = &symbol->GetUltimate(); - if (!symbol->owner().IsDerivedType() && !symbol->has() && - !symbol->has() && !IsObjectWithVisibleDSA(*symbol)) { + if (name.symbol && WithinConstruct()) { + const Symbol &symbol{name.symbol->GetUltimate()}; + if (!symbol.owner().IsDerivedType() && !symbol.has() && + !symbol.has() && !IsObjectWithVisibleDSA(symbol)) { if (Symbol * found{currScope().FindSymbol(name.source)}) { - if (symbol != found) { - name.symbol = found; // adjust the symbol within region + if (&symbol != found) { + // adjust the symbol within the region + // TODO: why didn't name resolution set the right name originally? + name.symbol = found; } else if (GetContext().defaultDSA == Symbol::Flag::AccNone) { // 2.5.14. context_.Say(name.source, "The DEFAULT(NONE) clause requires that '%s' must be listed in a data-mapping clause"_err_en_US, - symbol->name()); + symbol.name()); } + } else { + // TODO: assertion here? or clear name.symbol? } } - } // within OpenACC construct + } } Symbol *AccAttributeVisitor::ResolveAccCommonBlockName( @@ -1810,13 +1803,11 @@ Symbol *AccAttributeVisitor::ResolveAcc( Symbol *AccAttributeVisitor::DeclareOrMarkOtherAccessEntity( const parser::Name &name, Symbol::Flag accFlag) { - Symbol *prev{currScope().FindSymbol(name.source)}; - if (!name.symbol || !prev) { + if (name.symbol) { + return DeclareOrMarkOtherAccessEntity(*name.symbol, accFlag); + } else { return nullptr; - } else if (prev != name.symbol) { - name.symbol = prev; } - return DeclareOrMarkOtherAccessEntity(*prev, accFlag); } Symbol *AccAttributeVisitor::DeclareOrMarkOtherAccessEntity( @@ -2990,6 +2981,7 @@ void OmpAttributeVisitor::Post(const parser::Name &name) { } Symbol *OmpAttributeVisitor::ResolveName(const parser::Name *name) { + // TODO: why is the symbol not properly resolved by name resolution? if (auto *resolvedSymbol{ name ? GetContext().scope.FindSymbol(name->source) : nullptr}) { name->symbol = resolvedSymbol; diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp index 0af1c94502bb4..6319937dc3ddb 100644 --- a/flang/lib/Semantics/resolve-names.cpp +++ b/flang/lib/Semantics/resolve-names.cpp @@ -1441,6 +1441,30 @@ class AccVisitor : public virtual DeclarationVisitor { void Post(const parser::AccBeginLoopDirective &x) { messageHandler().set_currStmtSource(std::nullopt); } + bool Pre(const parser::OpenACCStandaloneConstruct &x) { + currScope().AddSourceRange(x.source); + return true; + } + bool Pre(const parser::OpenACCCacheConstruct &x) { + currScope().AddSourceRange(x.source); + return true; + } + bool Pre(const parser::OpenACCWaitConstruct &x) { + currScope().AddSourceRange(x.source); + return true; + } + bool Pre(const parser::OpenACCAtomicConstruct &x) { + currScope().AddSourceRange(x.source); + return true; + } + bool Pre(const parser::OpenACCEndConstruct &x) { + currScope().AddSourceRange(x.source); + return true; + } + bool Pre(const parser::OpenACCDeclarativeConstruct &x) { + currScope().AddSourceRange(x.source); + return true; + } void CopySymbolWithDevice(const parser::Name *name); @@ -1480,7 +1504,8 @@ void AccVisitor::CopySymbolWithDevice(const parser::Name *name) { // symbols are created for the one appearing in the use_device // clause. These new symbols have the CUDA Fortran device // attribute. - if (context_.languageFeatures().IsEnabled(common::LanguageFeature::CUDA)) { + if (context_.languageFeatures().IsEnabled(common::LanguageFeature::CUDA) && + name->symbol) { name->symbol = currScope().CopySymbol(*name->symbol); if (auto *object{name->symbol->detailsIf()}) { object->set_cudaDataAttr(common::CUDADataAttr::Device); @@ -1490,15 +1515,12 @@ void AccVisitor::CopySymbolWithDevice(const parser::Name *name) { bool AccVisitor::Pre(const parser::AccClause::UseDevice &x) { for (const auto &accObject : x.v.v) { + Walk(accObject); common::visit( common::visitors{ [&](const parser::Designator &designator) { if (const auto *name{ parser::GetDesignatorNameIfDataRef(designator)}) { - Symbol *prev{currScope().FindSymbol(name->source)}; - if (prev != name->symbol) { - name->symbol = prev; - } CopySymbolWithDevice(name); } else { if (const auto *dataRef{ @@ -1507,13 +1529,8 @@ bool AccVisitor::Pre(const parser::AccClause::UseDevice &x) { common::Indirection; if (auto *ind{std::get_if(&dataRef->u)}) { const parser::ArrayElement &arrayElement{ind->value()}; - Walk(arrayElement.subscripts); const parser::DataRef &base{arrayElement.base}; if (auto *name{std::get_if(&base.u)}) { - Symbol *prev{currScope().FindSymbol(name->source)}; - if (prev != name->symbol) { - name->symbol = prev; - } CopySymbolWithDevice(name); } } @@ -1537,6 +1554,7 @@ void AccVisitor::Post(const parser::OpenACCBlockConstruct &x) { bool AccVisitor::Pre(const parser::OpenACCCombinedConstruct &x) { PushScope(Scope::Kind::OpenACCConstruct, nullptr); + currScope().AddSourceRange(x.source); return true; } diff --git a/flang/lib/Semantics/semantics.cpp b/flang/lib/Semantics/semantics.cpp index bdb5377265c14..2606d997b1cd7 100644 --- a/flang/lib/Semantics/semantics.cpp +++ b/flang/lib/Semantics/semantics.cpp @@ -452,6 +452,15 @@ void SemanticsContext::UpdateScopeIndex( } } +void SemanticsContext::DumpScopeIndex(llvm::raw_ostream &out) const { + out << "scopeIndex_:\n"; + for (const auto &[source, scope] : scopeIndex_) { + out << "source '" << source.ToString() << "' -> scope " << scope + << "... whose source range is '" << scope.sourceRange().ToString() + << "'\n"; + } +} + bool SemanticsContext::IsInModuleFile(parser::CharBlock source) const { for (const Scope *scope{&FindScope(source)}; !scope->IsGlobal(); scope = &scope->parent()) { diff --git a/flang/test/Fir/OpenACC/openacc-type-categories-class.f90 b/flang/test/Fir/OpenACC/openacc-type-categories-class.f90 index e8951cceeeaeb..ec97114d30f88 100644 --- a/flang/test/Fir/OpenACC/openacc-type-categories-class.f90 +++ b/flang/test/Fir/OpenACC/openacc-type-categories-class.f90 @@ -43,4 +43,4 @@ subroutine init_unlimited(this) ! TODO: After using select type - the appropriate type category should be ! possible. Add the rest of the test once OpenACC lowering correctly handles -! unlimited polymorhic. +! unlimited polymorphic. diff --git a/flang/test/Semantics/OpenACC/bug1583.f90 b/flang/test/Semantics/OpenACC/bug1583.f90 new file mode 100644 index 0000000000000..7778d4644a9ff --- /dev/null +++ b/flang/test/Semantics/OpenACC/bug1583.f90 @@ -0,0 +1,23 @@ +! RUN: %python %S/../test_symbols.py %s %flang_fc1 -fopenacc +!DEF: /m Module +module m + !DEF: /m/t PUBLIC DerivedType + type :: t + !DEF: /m/t/c ALLOCATABLE ObjectEntity REAL(4) + real, allocatable :: c(:) + end type +contains + !DEF: /m/sub PUBLIC (Subroutine) Subprogram + !DEF: /m/sub/v ObjectEntity TYPE(t) + subroutine sub (v) + !REF: /m/t + !REF: /m/sub/v + type(t) :: v +!$acc host_data use_device(v%c) + !DEF: /foo EXTERNAL (Subroutine) ProcEntity + !REF: /m/sub/v + !REF: /m/t/c + call foo(v%c) +!$acc end host_data + end subroutine +end module