[Flang][OpenMP] Update declare mapper lookup via use-module #163860

TIFitis · 2025-10-16T20:42:11Z

Implemented semantic TODO to catch undeclared mappers.
Fix mapper lookup to include modules imported through USE.
Update and add tests.

Fixes #163385.

llvmbot · 2025-10-16T20:42:50Z

@llvm/pr-subscribers-flang-parser

@llvm/pr-subscribers-flang-fir-hlfir

Author: Akash Banerjee (TIFitis)

Changes

Implemented semantic TODO to catch undeclared mappers.
Fix mapper lookup to include modules imported through USE.
Update and add tests.

Fixes #163385.

Full diff: https://github.com/llvm/llvm-project/pull/163860.diff

5 Files Affected:

(modified) flang/lib/Lower/OpenMP/ClauseProcessor.cpp (+8-4)
(modified) flang/lib/Semantics/resolve-names.cpp (+21-10)
(modified) flang/test/Lower/OpenMP/declare-mapper.f90 (+24-1)
(modified) flang/test/Parser/OpenMP/map-modifiers.f90 (+3-4)
(modified) flang/test/Semantics/OpenMP/map-clause-symbols.f90 (+3-5)

diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp index 85398be778382..41820c74e3921 100644 --- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp +++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp @@ -1402,10 +1402,14 @@ bool ClauseProcessor::processMap( } if (mappers) { assert(mappers->size() == 1 && "more than one mapper"); - mapperIdName = mappers->front().v.id().symbol->name().ToString(); - if (mapperIdName != "default") - mapperIdName = converter.mangleName( - mapperIdName, mappers->front().v.id().symbol->owner()); + const semantics::Symbol *mapperSym = mappers->front().v.id().symbol; + mapperIdName = mapperSym->name().ToString(); + if (mapperIdName != "default") { + // Mangle with the ultimate owner so that use-associated mapper + // identifiers resolve to the same symbol as their defining scope. + const semantics::Symbol &ultimate = mapperSym->GetUltimate(); + mapperIdName = converter.mangleName(mapperIdName, ultimate.owner()); + } } processMapObjects(stmtCtx, clauseLocation, diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp index ae0ff9ca8068d..ef45c692acc8c 100644 --- a/flang/lib/Semantics/resolve-names.cpp +++ b/flang/lib/Semantics/resolve-names.cpp @@ -1824,21 +1824,24 @@ bool OmpVisitor::Pre(const parser::OmpMapClause &x) { // TODO: Do we need a specific flag or type here, to distinghuish against // other ConstructName things? Leaving this for the full implementation // of mapper lowering. - auto *misc{symbol->detailsIf<MiscDetails>()}; + const Symbol &ultimate{symbol->GetUltimate()}; + auto *misc{const_cast<Symbol &>(ultimate).detailsIf<MiscDetails>()}; if (!misc || misc->kind() != MiscDetails::Kind::ConstructName) context().Say(mapper->v.source, "Name '%s' should be a mapper name"_err_en_US, mapper->v.source); else mapper->v.symbol = symbol; } else { - mapper->v.symbol = - &MakeSymbol(mapper->v, MiscDetails{MiscDetails::Kind::ConstructName}); - // TODO: When completing the implementation, we probably want to error if - // the symbol is not declared, but right now, testing that the TODO for - // OmpMapClause happens is obscured by the TODO for declare mapper, so - // leaving this out. Remove the above line once the declare mapper is - // implemented. context().Say(mapper->v.source, "'%s' not - // declared"_err_en_US, mapper->v.source); + // Allow the special 'default' mapper identifier without prior + // declaration so lowering can recognize and handle it. Emit an + // error for any other missing mapper identifier. + if (mapper->v.source.ToString() == "default") { + mapper->v.symbol = &MakeSymbol( + mapper->v, MiscDetails{MiscDetails::Kind::ConstructName}); + } else { + context().Say( + mapper->v.source, "'%s' not declared"_err_en_US, mapper->v.source); + } } } return true; @@ -3614,10 +3617,18 @@ void ModuleVisitor::Post(const parser::UseStmt &x) { rename.u); } for (const auto &[name, symbol] : *useModuleScope_) { + // Default USE imports public names, excluding intrinsic-only and most + // miscellaneous details. However, allow OpenMP mapper identifiers, + // which are currently represented with MiscDetails::ConstructName. + bool isMapper{false}; + if (const auto *misc{symbol->detailsIf<MiscDetails>()}) { + isMapper = misc->kind() == MiscDetails::Kind::ConstructName; + } if (symbol->attrs().test(Attr::PUBLIC) && !IsUseRenamed(symbol->name()) && (!symbol->implicitAttrs().test(Attr::INTRINSIC) || symbol->has<UseDetails>()) && - !symbol->has<MiscDetails>() && useNames.count(name) == 0) { + (!symbol->has<MiscDetails>() || isMapper) && + useNames.count(name) == 0) { SourceName location{x.moduleName.source}; if (auto *localSymbol{FindInScope(name)}) { DoAddUse(location, localSymbol->name(), *localSymbol, *symbol); diff --git a/flang/test/Lower/OpenMP/declare-mapper.f90 b/flang/test/Lower/OpenMP/declare-mapper.f90 index 3d4d0da1e18a3..a5fb0710592ce 100644 --- a/flang/test/Lower/OpenMP/declare-mapper.f90 +++ b/flang/test/Lower/OpenMP/declare-mapper.f90 @@ -6,7 +6,8 @@ ! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-3.f90 -o - | FileCheck %t/omp-declare-mapper-3.f90 ! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-4.f90 -o - | FileCheck %t/omp-declare-mapper-4.f90 ! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-5.f90 -o - | FileCheck %t/omp-declare-mapper-5.f90 -! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=51 %t/omp-declare-mapper-6.f90 -o - | FileCheck %t/omp-declare-mapper-6.f90 +! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-6.f90 -o - | FileCheck %t/omp-declare-mapper-6.f90 +! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-7.f90 -o - | FileCheck %t/omp-declare-mapper-7.f90 !--- omp-declare-mapper-1.f90 subroutine declare_mapper_1 @@ -301,3 +302,25 @@ subroutine declare_mapper_nested_parent r%real_arr = r%base_arr(1) + r%inner%deep_arr(1) !$omp end target end subroutine declare_mapper_nested_parent + +!--- omp-declare-mapper-7.f90 +! Check mappers declared inside modules and used via USE association. +module m_mod + implicit none + type :: mty + integer :: x + end type mty + !$omp declare mapper(mymap : mty :: v) map(tofrom: v%x) +end module m_mod + +program use_module_mapper + use m_mod + implicit none + type(mty) :: a + + ! CHECK: omp.declare_mapper @[[MODMAP:[^ ]*mymap]] + ! CHECK: %{{.*}} = omp.map.info {{.*}} mapper(@[[MODMAP]]) {{.*}} {name = "a"} + !$omp target map(mapper(mymap) : a) + a%x = 42 + !$omp end target +end program use_module_mapper diff --git a/flang/test/Parser/OpenMP/map-modifiers.f90 b/flang/test/Parser/OpenMP/map-modifiers.f90 index 83662b70f08f5..7d9b8856ac833 100644 --- a/flang/test/Parser/OpenMP/map-modifiers.f90 +++ b/flang/test/Parser/OpenMP/map-modifiers.f90 @@ -320,7 +320,7 @@ subroutine f21(x, y) integer :: x(10) integer :: y integer, parameter :: p = 23 - !$omp target map(mapper(xx), from: x) + !$omp target map(mapper(default), from: x) x = x + 1 !$omp end target end @@ -329,7 +329,7 @@ subroutine f21(x, y) !UNPARSE: INTEGER x(10_4) !UNPARSE: INTEGER y !UNPARSE: INTEGER, PARAMETER :: p = 23_4 -!UNPARSE: !$OMP TARGET MAP(MAPPER(XX), FROM: X) +!UNPARSE: !$OMP TARGET MAP(MAPPER(DEFAULT), FROM: X) !UNPARSE: x=x+1_4 !UNPARSE: !$OMP END TARGET !UNPARSE: END SUBROUTINE @@ -337,7 +337,7 @@ subroutine f21(x, y) !PARSE-TREE: OmpBeginDirective !PARSE-TREE: | OmpDirectiveName -> llvm::omp::Directive = target !PARSE-TREE: | OmpClauseList -> OmpClause -> Map -> OmpMapClause -!PARSE-TREE: | | Modifier -> OmpMapper -> Name = 'xx' +!PARSE-TREE: | | Modifier -> OmpMapper -> Name = 'default' !PARSE-TREE: | | Modifier -> OmpMapType -> Value = From !PARSE-TREE: | | OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'x' @@ -375,4 +375,3 @@ subroutine f22(x) !PARSE-TREE: | | SectionSubscript -> Integer -> Expr = 'i' !PARSE-TREE: | | | Designator -> DataRef -> Name = 'i' !PARSE-TREE: | bool = 'true' - diff --git a/flang/test/Semantics/OpenMP/map-clause-symbols.f90 b/flang/test/Semantics/OpenMP/map-clause-symbols.f90 index 1d6315b4a2312..cdb65a71e8887 100644 --- a/flang/test/Semantics/OpenMP/map-clause-symbols.f90 +++ b/flang/test/Semantics/OpenMP/map-clause-symbols.f90 @@ -1,6 +1,5 @@ -! RUN: %flang_fc1 -fdebug-dump-symbols -fopenmp -fopenmp-version=50 %s | FileCheck %s +! RUN: not %flang_fc1 -fopenmp -fopenmp-version=50 %s 2>&1 | FileCheck %s program main -!CHECK-LABEL: MainProgram scope: MAIN integer, parameter :: n = 256 real(8) :: a(256) !$omp target map(mapper(xx), from:a) @@ -8,7 +7,6 @@ program main a(i) = 4.2 end do !$omp end target -!CHECK: OtherConstruct scope: size=0 alignment=1 sourceRange=74 bytes -!CHECK: OtherClause scope: size=0 alignment=1 sourceRange=0 bytes -!CHECK: xx: Misc ConstructName end program main + +! CHECK: error: '{{.*}}' not declared

llvmbot · 2025-10-16T20:42:51Z

@llvm/pr-subscribers-flang-openmp

Author: Akash Banerjee (TIFitis)

Changes

Implemented semantic TODO to catch undeclared mappers.
Fix mapper lookup to include modules imported through USE.
Update and add tests.

Fixes #163385.

Full diff: https://github.com/llvm/llvm-project/pull/163860.diff

5 Files Affected:

(modified) flang/lib/Lower/OpenMP/ClauseProcessor.cpp (+8-4)
(modified) flang/lib/Semantics/resolve-names.cpp (+21-10)
(modified) flang/test/Lower/OpenMP/declare-mapper.f90 (+24-1)
(modified) flang/test/Parser/OpenMP/map-modifiers.f90 (+3-4)
(modified) flang/test/Semantics/OpenMP/map-clause-symbols.f90 (+3-5)

diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp index 85398be778382..41820c74e3921 100644 --- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp +++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp @@ -1402,10 +1402,14 @@ bool ClauseProcessor::processMap( } if (mappers) { assert(mappers->size() == 1 && "more than one mapper"); - mapperIdName = mappers->front().v.id().symbol->name().ToString(); - if (mapperIdName != "default") - mapperIdName = converter.mangleName( - mapperIdName, mappers->front().v.id().symbol->owner()); + const semantics::Symbol *mapperSym = mappers->front().v.id().symbol; + mapperIdName = mapperSym->name().ToString(); + if (mapperIdName != "default") { + // Mangle with the ultimate owner so that use-associated mapper + // identifiers resolve to the same symbol as their defining scope. + const semantics::Symbol &ultimate = mapperSym->GetUltimate(); + mapperIdName = converter.mangleName(mapperIdName, ultimate.owner()); + } } processMapObjects(stmtCtx, clauseLocation, diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp index ae0ff9ca8068d..ef45c692acc8c 100644 --- a/flang/lib/Semantics/resolve-names.cpp +++ b/flang/lib/Semantics/resolve-names.cpp @@ -1824,21 +1824,24 @@ bool OmpVisitor::Pre(const parser::OmpMapClause &x) { // TODO: Do we need a specific flag or type here, to distinghuish against // other ConstructName things? Leaving this for the full implementation // of mapper lowering. - auto *misc{symbol->detailsIf<MiscDetails>()}; + const Symbol &ultimate{symbol->GetUltimate()}; + auto *misc{const_cast<Symbol &>(ultimate).detailsIf<MiscDetails>()}; if (!misc || misc->kind() != MiscDetails::Kind::ConstructName) context().Say(mapper->v.source, "Name '%s' should be a mapper name"_err_en_US, mapper->v.source); else mapper->v.symbol = symbol; } else { - mapper->v.symbol = - &MakeSymbol(mapper->v, MiscDetails{MiscDetails::Kind::ConstructName}); - // TODO: When completing the implementation, we probably want to error if - // the symbol is not declared, but right now, testing that the TODO for - // OmpMapClause happens is obscured by the TODO for declare mapper, so - // leaving this out. Remove the above line once the declare mapper is - // implemented. context().Say(mapper->v.source, "'%s' not - // declared"_err_en_US, mapper->v.source); + // Allow the special 'default' mapper identifier without prior + // declaration so lowering can recognize and handle it. Emit an + // error for any other missing mapper identifier. + if (mapper->v.source.ToString() == "default") { + mapper->v.symbol = &MakeSymbol( + mapper->v, MiscDetails{MiscDetails::Kind::ConstructName}); + } else { + context().Say( + mapper->v.source, "'%s' not declared"_err_en_US, mapper->v.source); + } } } return true; @@ -3614,10 +3617,18 @@ void ModuleVisitor::Post(const parser::UseStmt &x) { rename.u); } for (const auto &[name, symbol] : *useModuleScope_) { + // Default USE imports public names, excluding intrinsic-only and most + // miscellaneous details. However, allow OpenMP mapper identifiers, + // which are currently represented with MiscDetails::ConstructName. + bool isMapper{false}; + if (const auto *misc{symbol->detailsIf<MiscDetails>()}) { + isMapper = misc->kind() == MiscDetails::Kind::ConstructName; + } if (symbol->attrs().test(Attr::PUBLIC) && !IsUseRenamed(symbol->name()) && (!symbol->implicitAttrs().test(Attr::INTRINSIC) || symbol->has<UseDetails>()) && - !symbol->has<MiscDetails>() && useNames.count(name) == 0) { + (!symbol->has<MiscDetails>() || isMapper) && + useNames.count(name) == 0) { SourceName location{x.moduleName.source}; if (auto *localSymbol{FindInScope(name)}) { DoAddUse(location, localSymbol->name(), *localSymbol, *symbol); diff --git a/flang/test/Lower/OpenMP/declare-mapper.f90 b/flang/test/Lower/OpenMP/declare-mapper.f90 index 3d4d0da1e18a3..a5fb0710592ce 100644 --- a/flang/test/Lower/OpenMP/declare-mapper.f90 +++ b/flang/test/Lower/OpenMP/declare-mapper.f90 @@ -6,7 +6,8 @@ ! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-3.f90 -o - | FileCheck %t/omp-declare-mapper-3.f90 ! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-4.f90 -o - | FileCheck %t/omp-declare-mapper-4.f90 ! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-5.f90 -o - | FileCheck %t/omp-declare-mapper-5.f90 -! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=51 %t/omp-declare-mapper-6.f90 -o - | FileCheck %t/omp-declare-mapper-6.f90 +! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-6.f90 -o - | FileCheck %t/omp-declare-mapper-6.f90 +! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-7.f90 -o - | FileCheck %t/omp-declare-mapper-7.f90 !--- omp-declare-mapper-1.f90 subroutine declare_mapper_1 @@ -301,3 +302,25 @@ subroutine declare_mapper_nested_parent r%real_arr = r%base_arr(1) + r%inner%deep_arr(1) !$omp end target end subroutine declare_mapper_nested_parent + +!--- omp-declare-mapper-7.f90 +! Check mappers declared inside modules and used via USE association. +module m_mod + implicit none + type :: mty + integer :: x + end type mty + !$omp declare mapper(mymap : mty :: v) map(tofrom: v%x) +end module m_mod + +program use_module_mapper + use m_mod + implicit none + type(mty) :: a + + ! CHECK: omp.declare_mapper @[[MODMAP:[^ ]*mymap]] + ! CHECK: %{{.*}} = omp.map.info {{.*}} mapper(@[[MODMAP]]) {{.*}} {name = "a"} + !$omp target map(mapper(mymap) : a) + a%x = 42 + !$omp end target +end program use_module_mapper diff --git a/flang/test/Parser/OpenMP/map-modifiers.f90 b/flang/test/Parser/OpenMP/map-modifiers.f90 index 83662b70f08f5..7d9b8856ac833 100644 --- a/flang/test/Parser/OpenMP/map-modifiers.f90 +++ b/flang/test/Parser/OpenMP/map-modifiers.f90 @@ -320,7 +320,7 @@ subroutine f21(x, y) integer :: x(10) integer :: y integer, parameter :: p = 23 - !$omp target map(mapper(xx), from: x) + !$omp target map(mapper(default), from: x) x = x + 1 !$omp end target end @@ -329,7 +329,7 @@ subroutine f21(x, y) !UNPARSE: INTEGER x(10_4) !UNPARSE: INTEGER y !UNPARSE: INTEGER, PARAMETER :: p = 23_4 -!UNPARSE: !$OMP TARGET MAP(MAPPER(XX), FROM: X) +!UNPARSE: !$OMP TARGET MAP(MAPPER(DEFAULT), FROM: X) !UNPARSE: x=x+1_4 !UNPARSE: !$OMP END TARGET !UNPARSE: END SUBROUTINE @@ -337,7 +337,7 @@ subroutine f21(x, y) !PARSE-TREE: OmpBeginDirective !PARSE-TREE: | OmpDirectiveName -> llvm::omp::Directive = target !PARSE-TREE: | OmpClauseList -> OmpClause -> Map -> OmpMapClause -!PARSE-TREE: | | Modifier -> OmpMapper -> Name = 'xx' +!PARSE-TREE: | | Modifier -> OmpMapper -> Name = 'default' !PARSE-TREE: | | Modifier -> OmpMapType -> Value = From !PARSE-TREE: | | OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'x' @@ -375,4 +375,3 @@ subroutine f22(x) !PARSE-TREE: | | SectionSubscript -> Integer -> Expr = 'i' !PARSE-TREE: | | | Designator -> DataRef -> Name = 'i' !PARSE-TREE: | bool = 'true' - diff --git a/flang/test/Semantics/OpenMP/map-clause-symbols.f90 b/flang/test/Semantics/OpenMP/map-clause-symbols.f90 index 1d6315b4a2312..cdb65a71e8887 100644 --- a/flang/test/Semantics/OpenMP/map-clause-symbols.f90 +++ b/flang/test/Semantics/OpenMP/map-clause-symbols.f90 @@ -1,6 +1,5 @@ -! RUN: %flang_fc1 -fdebug-dump-symbols -fopenmp -fopenmp-version=50 %s | FileCheck %s +! RUN: not %flang_fc1 -fopenmp -fopenmp-version=50 %s 2>&1 | FileCheck %s program main -!CHECK-LABEL: MainProgram scope: MAIN integer, parameter :: n = 256 real(8) :: a(256) !$omp target map(mapper(xx), from:a) @@ -8,7 +7,6 @@ program main a(i) = 4.2 end do !$omp end target -!CHECK: OtherConstruct scope: size=0 alignment=1 sourceRange=74 bytes -!CHECK: OtherClause scope: size=0 alignment=1 sourceRange=0 bytes -!CHECK: xx: Misc ConstructName end program main + +! CHECK: error: '{{.*}}' not declared

llvmbot · 2025-10-16T20:42:51Z

@llvm/pr-subscribers-flang-semantics

Author: Akash Banerjee (TIFitis)

Changes

Implemented semantic TODO to catch undeclared mappers.
Fix mapper lookup to include modules imported through USE.
Update and add tests.

Fixes #163385.

Full diff: https://github.com/llvm/llvm-project/pull/163860.diff

5 Files Affected:

(modified) flang/lib/Lower/OpenMP/ClauseProcessor.cpp (+8-4)
(modified) flang/lib/Semantics/resolve-names.cpp (+21-10)
(modified) flang/test/Lower/OpenMP/declare-mapper.f90 (+24-1)
(modified) flang/test/Parser/OpenMP/map-modifiers.f90 (+3-4)
(modified) flang/test/Semantics/OpenMP/map-clause-symbols.f90 (+3-5)

diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp index 85398be778382..41820c74e3921 100644 --- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp +++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp @@ -1402,10 +1402,14 @@ bool ClauseProcessor::processMap( } if (mappers) { assert(mappers->size() == 1 && "more than one mapper"); - mapperIdName = mappers->front().v.id().symbol->name().ToString(); - if (mapperIdName != "default") - mapperIdName = converter.mangleName( - mapperIdName, mappers->front().v.id().symbol->owner()); + const semantics::Symbol *mapperSym = mappers->front().v.id().symbol; + mapperIdName = mapperSym->name().ToString(); + if (mapperIdName != "default") { + // Mangle with the ultimate owner so that use-associated mapper + // identifiers resolve to the same symbol as their defining scope. + const semantics::Symbol &ultimate = mapperSym->GetUltimate(); + mapperIdName = converter.mangleName(mapperIdName, ultimate.owner()); + } } processMapObjects(stmtCtx, clauseLocation, diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp index ae0ff9ca8068d..ef45c692acc8c 100644 --- a/flang/lib/Semantics/resolve-names.cpp +++ b/flang/lib/Semantics/resolve-names.cpp @@ -1824,21 +1824,24 @@ bool OmpVisitor::Pre(const parser::OmpMapClause &x) { // TODO: Do we need a specific flag or type here, to distinghuish against // other ConstructName things? Leaving this for the full implementation // of mapper lowering. - auto *misc{symbol->detailsIf<MiscDetails>()}; + const Symbol &ultimate{symbol->GetUltimate()}; + auto *misc{const_cast<Symbol &>(ultimate).detailsIf<MiscDetails>()}; if (!misc || misc->kind() != MiscDetails::Kind::ConstructName) context().Say(mapper->v.source, "Name '%s' should be a mapper name"_err_en_US, mapper->v.source); else mapper->v.symbol = symbol; } else { - mapper->v.symbol = - &MakeSymbol(mapper->v, MiscDetails{MiscDetails::Kind::ConstructName}); - // TODO: When completing the implementation, we probably want to error if - // the symbol is not declared, but right now, testing that the TODO for - // OmpMapClause happens is obscured by the TODO for declare mapper, so - // leaving this out. Remove the above line once the declare mapper is - // implemented. context().Say(mapper->v.source, "'%s' not - // declared"_err_en_US, mapper->v.source); + // Allow the special 'default' mapper identifier without prior + // declaration so lowering can recognize and handle it. Emit an + // error for any other missing mapper identifier. + if (mapper->v.source.ToString() == "default") { + mapper->v.symbol = &MakeSymbol( + mapper->v, MiscDetails{MiscDetails::Kind::ConstructName}); + } else { + context().Say( + mapper->v.source, "'%s' not declared"_err_en_US, mapper->v.source); + } } } return true; @@ -3614,10 +3617,18 @@ void ModuleVisitor::Post(const parser::UseStmt &x) { rename.u); } for (const auto &[name, symbol] : *useModuleScope_) { + // Default USE imports public names, excluding intrinsic-only and most + // miscellaneous details. However, allow OpenMP mapper identifiers, + // which are currently represented with MiscDetails::ConstructName. + bool isMapper{false}; + if (const auto *misc{symbol->detailsIf<MiscDetails>()}) { + isMapper = misc->kind() == MiscDetails::Kind::ConstructName; + } if (symbol->attrs().test(Attr::PUBLIC) && !IsUseRenamed(symbol->name()) && (!symbol->implicitAttrs().test(Attr::INTRINSIC) || symbol->has<UseDetails>()) && - !symbol->has<MiscDetails>() && useNames.count(name) == 0) { + (!symbol->has<MiscDetails>() || isMapper) && + useNames.count(name) == 0) { SourceName location{x.moduleName.source}; if (auto *localSymbol{FindInScope(name)}) { DoAddUse(location, localSymbol->name(), *localSymbol, *symbol); diff --git a/flang/test/Lower/OpenMP/declare-mapper.f90 b/flang/test/Lower/OpenMP/declare-mapper.f90 index 3d4d0da1e18a3..a5fb0710592ce 100644 --- a/flang/test/Lower/OpenMP/declare-mapper.f90 +++ b/flang/test/Lower/OpenMP/declare-mapper.f90 @@ -6,7 +6,8 @@ ! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-3.f90 -o - | FileCheck %t/omp-declare-mapper-3.f90 ! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-4.f90 -o - | FileCheck %t/omp-declare-mapper-4.f90 ! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-5.f90 -o - | FileCheck %t/omp-declare-mapper-5.f90 -! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=51 %t/omp-declare-mapper-6.f90 -o - | FileCheck %t/omp-declare-mapper-6.f90 +! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-6.f90 -o - | FileCheck %t/omp-declare-mapper-6.f90 +! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/omp-declare-mapper-7.f90 -o - | FileCheck %t/omp-declare-mapper-7.f90 !--- omp-declare-mapper-1.f90 subroutine declare_mapper_1 @@ -301,3 +302,25 @@ subroutine declare_mapper_nested_parent r%real_arr = r%base_arr(1) + r%inner%deep_arr(1) !$omp end target end subroutine declare_mapper_nested_parent + +!--- omp-declare-mapper-7.f90 +! Check mappers declared inside modules and used via USE association. +module m_mod + implicit none + type :: mty + integer :: x + end type mty + !$omp declare mapper(mymap : mty :: v) map(tofrom: v%x) +end module m_mod + +program use_module_mapper + use m_mod + implicit none + type(mty) :: a + + ! CHECK: omp.declare_mapper @[[MODMAP:[^ ]*mymap]] + ! CHECK: %{{.*}} = omp.map.info {{.*}} mapper(@[[MODMAP]]) {{.*}} {name = "a"} + !$omp target map(mapper(mymap) : a) + a%x = 42 + !$omp end target +end program use_module_mapper diff --git a/flang/test/Parser/OpenMP/map-modifiers.f90 b/flang/test/Parser/OpenMP/map-modifiers.f90 index 83662b70f08f5..7d9b8856ac833 100644 --- a/flang/test/Parser/OpenMP/map-modifiers.f90 +++ b/flang/test/Parser/OpenMP/map-modifiers.f90 @@ -320,7 +320,7 @@ subroutine f21(x, y) integer :: x(10) integer :: y integer, parameter :: p = 23 - !$omp target map(mapper(xx), from: x) + !$omp target map(mapper(default), from: x) x = x + 1 !$omp end target end @@ -329,7 +329,7 @@ subroutine f21(x, y) !UNPARSE: INTEGER x(10_4) !UNPARSE: INTEGER y !UNPARSE: INTEGER, PARAMETER :: p = 23_4 -!UNPARSE: !$OMP TARGET MAP(MAPPER(XX), FROM: X) +!UNPARSE: !$OMP TARGET MAP(MAPPER(DEFAULT), FROM: X) !UNPARSE: x=x+1_4 !UNPARSE: !$OMP END TARGET !UNPARSE: END SUBROUTINE @@ -337,7 +337,7 @@ subroutine f21(x, y) !PARSE-TREE: OmpBeginDirective !PARSE-TREE: | OmpDirectiveName -> llvm::omp::Directive = target !PARSE-TREE: | OmpClauseList -> OmpClause -> Map -> OmpMapClause -!PARSE-TREE: | | Modifier -> OmpMapper -> Name = 'xx' +!PARSE-TREE: | | Modifier -> OmpMapper -> Name = 'default' !PARSE-TREE: | | Modifier -> OmpMapType -> Value = From !PARSE-TREE: | | OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'x' @@ -375,4 +375,3 @@ subroutine f22(x) !PARSE-TREE: | | SectionSubscript -> Integer -> Expr = 'i' !PARSE-TREE: | | | Designator -> DataRef -> Name = 'i' !PARSE-TREE: | bool = 'true' - diff --git a/flang/test/Semantics/OpenMP/map-clause-symbols.f90 b/flang/test/Semantics/OpenMP/map-clause-symbols.f90 index 1d6315b4a2312..cdb65a71e8887 100644 --- a/flang/test/Semantics/OpenMP/map-clause-symbols.f90 +++ b/flang/test/Semantics/OpenMP/map-clause-symbols.f90 @@ -1,6 +1,5 @@ -! RUN: %flang_fc1 -fdebug-dump-symbols -fopenmp -fopenmp-version=50 %s | FileCheck %s +! RUN: not %flang_fc1 -fopenmp -fopenmp-version=50 %s 2>&1 | FileCheck %s program main -!CHECK-LABEL: MainProgram scope: MAIN integer, parameter :: n = 256 real(8) :: a(256) !$omp target map(mapper(xx), from:a) @@ -8,7 +7,6 @@ program main a(i) = 4.2 end do !$omp end target -!CHECK: OtherConstruct scope: size=0 alignment=1 sourceRange=74 bytes -!CHECK: OtherClause scope: size=0 alignment=1 sourceRange=0 bytes -!CHECK: xx: Misc ConstructName end program main + +! CHECK: error: '{{.*}}' not declared

Copilot

Pull Request Overview

This PR fixes the handling of OpenMP declare mapper identifiers when they are imported via USE statements from modules. The changes address issue #163385 by implementing proper semantic checking for undeclared mappers and ensuring mapper lookup includes module-imported identifiers.

Key Changes

Implemented semantic error checking to catch undeclared mapper identifiers (except for the special "default" mapper)
Fixed mapper symbol resolution to use the ultimate symbol owner for proper mangling when mappers are imported via USE
Added support for importing OpenMP mapper identifiers through module USE statements

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
flang/test/Semantics/OpenMP/map-clause-symbols.f90	Updated test to verify error reporting for undeclared mapper identifiers
flang/test/Parser/OpenMP/map-modifiers.f90	Changed test to use "default" mapper instead of undeclared "xx" mapper
flang/test/Lower/OpenMP/declare-mapper.f90	Added new test case for mapper usage via module USE association
flang/lib/Semantics/resolve-names.cpp	Implemented error checking for undeclared mappers and enabled mapper import through USE statements
flang/lib/Lower/OpenMP/ClauseProcessor.cpp	Fixed mapper name mangling to use ultimate symbol owner for use-associated mappers

flang/lib/Semantics/resolve-names.cpp

flang/lib/Lower/OpenMP/ClauseProcessor.cpp

kiranchandramohan · 2025-10-21T13:34:40Z

I don't see changes to add support (in lib/Semantics/mod-file.cpp) for emitting the declare mapper directive in the module file (*.mod). How did you manage to get this to work without it?

TIFitis · 2025-10-21T14:54:49Z

I don't see changes to add support (in lib/Semantics/mod-file.cpp) for emitting the declare mapper directive in the module file (*.mod). How did you manage to get this to work without it?

I think it gets added here -

llvm-project/flang/lib/Lower/OpenMP/OpenMP.cpp

Line 3583 in 48a99ad

firOpBuilder.setInsertionPointToStart(converter.getModuleOp().getBody());

TIFitis · 2025-10-22T12:52:20Z

Ping for review.

kiranchandramohan · 2025-10-22T13:02:15Z

I don't see changes to add support (in lib/Semantics/mod-file.cpp) for emitting the declare mapper directive in the module file (*.mod). How did you manage to get this to work without it?

I think it gets added here -

llvm-project/flang/lib/Lower/OpenMP/OpenMP.cpp

Line 3583 in 48a99ad

firOpBuilder.setInsertionPointToStart(converter.getModuleOp().getBody());

I meant the module file (m_mod.mod) should have an entry for the declare mapper. Without this how will the mapper in !$omp target map(mapper(mymap) : a) know about mymap?

!mod$ v1 sum:bf468214a8672562 module m_mod type::mty integer(4)::x end type !$omp declare mapper(mymap : mty :: v) map(tofrom: v%x) end

Does the FIR/OMP code generation generate a dummy declare mapper if none is found? If so that is not complete.

TIFitis · 2025-10-22T21:41:15Z

@kiranchandramohan Sorry for the earlier misunderstanding — I’m not very familiar with the parsing/semantic side of things.

It was working without exporting to the .mod file because the test originally had both the module and the program in the same file. Once separated, it started failing.

I’ve now updated the PR to export the declare mapper to the .mod file as you suggested, and the lookup works correctly when compiling across different files. I’ve also updated the test accordingly.

As far as I’m aware, FIR/OMP does not emit dummy mappers at any stage.

kiranchandramohan · 2025-10-24T09:18:20Z

@kiranchandramohan Sorry for the earlier misunderstanding — I’m not very familiar with the parsing/semantic side of things.

It was working without exporting to the .mod file because the test originally had both the module and the program in the same file. Once separated, it started failing.

I’ve now updated the PR to export the declare mapper to the .mod file as you suggested, and the lookup works correctly when compiling across different files. I’ve also updated the test accordingly.

As far as I’m aware, FIR/OMP does not emit dummy mappers at any stage.

Thanks for the updates. I have a question and two requests for tests.

flang/test/Semantics/OpenMP/map-clause-symbols.f90

flang/lib/Semantics/mod-file.cpp

kiranchandramohan · 2025-10-24T09:15:52Z

flang/lib/Lower/OpenMP/OpenMP.cpp

+// Walk scopes and materialize omp.declare_mapper ops for mapper declarations
+// found in imported modules. If \p scope is null, start from the global scope.
+void Fortran::lower::materializeOpenMPDeclareMappers(
+ Fortran::lower::AbstractConverter &converter,
+ semantics::SemanticsContext &semaCtx, const semantics::Scope *scope) {
+ const semantics::Scope &root = scope ? *scope : semaCtx.globalScope();
+
+ // Recurse into child scopes first (modules, submodules, etc.).
+ for (const semantics::Scope &child : root.children())
+ materializeOpenMPDeclareMappers(converter, semaCtx, &child);
+
+ // Only consider module scopes to avoid duplicating local constructs.
+ if (!root.IsModule())
+ return;
+
+ // Scan symbols in this module scope for MapperDetails.
+ for (auto &it : root) {
+ const semantics::Symbol &sym = *it.second;
+ if (auto *md = sym.detailsIf<semantics::MapperDetails>()) {
+ for (const auto *decl : md->GetDeclList()) {
+ if (const auto *mapperDecl =
+ std::get_if<parser::OpenMPDeclareMapperConstruct>(&decl->u)) {
+ genOpenMPDeclareMapperImpl(converter, semaCtx, *mapperDecl, &sym);
+ }
+ }
+ }
+ }
+}


Do we still need this function? Can you check how threadprivate works and whether it needs this kind of handling?

Threadprivate directive is serialised into the .mod, and we don't need to lookup symbols by name. We need to lookup existing declare mappers in the module when they are referred to through mapper modifiers, as such, I think we need the materializeOpenMPDeclareMappers function.

Do we need to recursively go through all the scopes? Or are only the declare mappers at the top-level needed?

If X uses module A and A USEs module B (B from a .mod), B’s mappers will live under A’s scope in the semantics tree. Without recursion, B’s mappers would be skipped, and map(mapper(...)) using B’s mapper would not resolve.

Otherwise, we already early-return for local scopes and non .mod files.

Could you help me understand, how is this different from emitting declarations for functions or module variables? And why is it sufficient for module functions but a recursive traversal required for declare mappers?

I've changed the approach we use to define the inherited declare mappers. Instead of defining them as a pre-process step, I've switched it to a lazy approach when the mapper is referred in the body.

Let me know if you're happy with this approach.

I wasn't requesting you to change the approach here. I was trying to understand why we needed additional recursive handling for the declare mapper directives compared to module functions.

I think the previous method of eagerly generating declare mappers is more appropriate than lazy generation. Unless you are convinced that eager generation is not correct.

I've reverted to the previous approach.

From what I understand, a function that comes in through a transitive USE is represented in the importing scope by a symbol whose details are semantics::UseDetails, pointing back to the defining symbol in the original module. When lowering sees a call, it follows that chain to the ultimate symbol in the source module, so the call is generated exactly as if the function were imported directly. No IR for the callee is emitted in the current compilation unit.

For declare mappers, we need to follow the chain and generate the IR. So we can either take the lazy approach, or we must go through the scopes to find any declare mappers that have brought in through USE modules and lower them.

The declare mappers don't seem to appear at the top-level.

Copilot

Pull Request Overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

flang/lib/Lower/OpenMP/OpenMP.cpp

kiranchandramohan · 2025-10-28T21:18:37Z

flang/lib/Lower/Bridge.cpp

+ // Ensure imported OpenMP declare mappers are materialized at module
+ // scope before lowering any constructs that may reference them.
+ createBuilderOutsideOfFuncOpAndDo([&]() {
+ Fortran::lower::materializeOpenMPDeclareMappers(
+ *this, bridge.getSemanticsContext());
+ });
+


Does the createBuilderOutsideOfFuncOpAndDo function above this visit all the top level OpenMP directives? if so is it possible to move this there and just create the Declare Mapper operation while visiting OpenMP directives?

I think we can't move this above for the same reason stated above.

github-actions · 2025-10-29T19:54:32Z

✅ With the latest revision this PR passed the C/C++ code formatter.

- Implemented semantic TODO to catch undeclared mappers. - Fix mapper lookup to include modules imported through USE. - Update and add tests. Fixes llvm#163385.

…e and program file compilation.

… .mod file.

kiranchandramohan

I will be away for atleast a week. This needs further review from @agozillon @skatrak or @kparzysz

Approving to signal that I am not blocking the progress of the patch.

TIFitis · 2025-11-06T21:33:54Z

I will be away for atleast a week. This needs further review from @agozillon @skatrak or @kparzysz

Approving to signal that I am not blocking the progress of the patch.

Thanks for the review 👍🏽

TIFitis · 2025-11-11T13:49:34Z

Ping for review.

kparzysz · 2025-11-12T17:03:14Z

The semantic parts look ok to me. Presumably we would do the same thing for user-declared reductions in terms of code generation. @jsjodin: does this look like what you're doing?

kparzysz

This looks reasonable. If some parts turn out to be unnecessary, we can remove them later.

…163860)" This reverts commit bb5f3a0.

…167896) Reverts #163860

…se-module" (#167896) Reverts llvm/llvm-project#163860

TIFitis requested review from Copilot, kiranchandramohan and kparzysz October 16, 2025 20:42

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp flang:semantics flang:parser labels Oct 16, 2025

Copilot AI reviewed Oct 16, 2025

View reviewed changes

flang/lib/Semantics/resolve-names.cpp Outdated Show resolved Hide resolved

flang/lib/Lower/OpenMP/ClauseProcessor.cpp Show resolved Hide resolved

TIFitis requested review from Stylie777 and tblah October 20, 2025 13:29

kiranchandramohan reviewed Oct 24, 2025

View reviewed changes

TIFitis requested a review from Copilot October 24, 2025 18:35

Copilot AI reviewed Oct 24, 2025

View reviewed changes

flang/lib/Lower/OpenMP/OpenMP.cpp Outdated Show resolved Hide resolved

flang/lib/Lower/OpenMP/OpenMP.cpp Outdated Show resolved Hide resolved

TIFitis requested a review from kiranchandramohan October 28, 2025 14:21

kiranchandramohan requested review from agozillon and skatrak October 28, 2025 14:37

kiranchandramohan reviewed Oct 28, 2025

View reviewed changes

TIFitis added 2 commits October 30, 2025 14:57

[Flang][OpenMP] Update declare mapper lookup via use-module

f56f6c2

- Implemented semantic TODO to catch undeclared mappers. - Fix mapper lookup to include modules imported through USE. - Update and add tests. Fixes llvm#163385.

code cleanup

117d77e

TIFitis added 3 commits October 30, 2025 14:57

Emit declare_mapper to .mod file. Update test to check separate modul…

6e23352

…e and program file compilation.

Address reviewer comments. Add test to check declare mapper export to…

672eb0e

… .mod file.

Address copilot comments.

83ab55c

TIFitis force-pushed the mapper_use_module branch from e98f7a7 to 83b7885 Compare October 30, 2025 15:01

TIFitis requested a review from kiranchandramohan November 3, 2025 13:51

TIFitis force-pushed the mapper_use_module branch from 83b7885 to 83ab55c Compare November 4, 2025 07:39

kiranchandramohan approved these changes Nov 6, 2025

View reviewed changes

kparzysz approved these changes Nov 12, 2025

View reviewed changes

TIFitis merged commit bb5f3a0 into llvm:main Nov 13, 2025
10 checks passed

TIFitis added a commit that referenced this pull request Nov 13, 2025

Revert "[Flang][OpenMP] Update declare mapper lookup via use-module (#…

6e01340

…163860)" This reverts commit bb5f3a0.

TIFitis mentioned this pull request Nov 13, 2025

Revert "[Flang][OpenMP] Update declare mapper lookup via use-module" #167896

Merged

TIFitis added a commit that referenced this pull request Nov 13, 2025

Revert "[Flang][OpenMP] Update declare mapper lookup via use-module" (#…

e1324a9

…167896) Reverts #163860

llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Nov 13, 2025

Automerge: Revert "[Flang][OpenMP] Update declare mapper lookup via u…

71bcdc4

…se-module" (#167896) Reverts llvm/llvm-project#163860

[Flang][OpenMP] Update declare mapper lookup via use-module #163860

[Flang][OpenMP] Update declare mapper lookup via use-module #163860

Uh oh!

Conversation

TIFitis commented Oct 16, 2025

llvmbot commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

llvmbot commented Oct 16, 2025

llvmbot commented Oct 16, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Uh oh!

kiranchandramohan commented Oct 21, 2025

TIFitis commented Oct 21, 2025

TIFitis commented Oct 22, 2025

kiranchandramohan commented Oct 22, 2025

TIFitis commented Oct 22, 2025

kiranchandramohan commented Oct 24, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

kiranchandramohan left a comment

Choose a reason for hiding this comment

TIFitis commented Nov 6, 2025

TIFitis commented Nov 11, 2025

kparzysz commented Nov 12, 2025

kparzysz left a comment

Choose a reason for hiding this comment

Uh oh!

Labels

4 participants

llvmbot commented Oct 16, 2025 •

edited

Loading

github-actions bot commented Oct 29, 2025 •

edited

Loading