From a4ba0a2f3b2cbec69ec91623af7f5eb228d1ac61 Mon Sep 17 00:00:00 2001 From: Mats Petersson Date: Wed, 13 Nov 2024 16:40:11 +0000 Subject: [PATCH 1/3] [flang][OpenMP]Add parsing support for MAP(MAPPER(name) ...) This prepares for using the DECLARE MAPPER construct. A check in lowering will say "Not implemented" when trying to use a mapper as some code is required to tie the mapper to the declared one. Senantics check for the symbol generated. --- flang/include/flang/Parser/dump-parse-tree.h | 1 + flang/include/flang/Parser/parse-tree.h | 8 ++++-- flang/lib/Lower/OpenMP/Clauses.cpp | 5 ++++ flang/lib/Parser/openmp-parsers.cpp | 18 +++++++------ flang/lib/Parser/unparse.cpp | 10 ++++++++ flang/lib/Semantics/resolve-names.cpp | 10 ++++++++ flang/test/Lower/OpenMP/Todo/map-mapper.f90 | 11 ++++++++ flang/test/Parser/OpenMP/map-modifiers.f90 | 25 +++++++++++++++++++ .../Semantics/OpenMP/map-clause-symbols.f90 | 14 +++++++++++ 9 files changed, 93 insertions(+), 9 deletions(-) create mode 100644 flang/test/Lower/OpenMP/Todo/map-mapper.f90 create mode 100644 flang/test/Semantics/OpenMP/map-clause-symbols.f90 diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h index 5886e384b986b..520b7ceeb1449 100644 --- a/flang/include/flang/Parser/dump-parse-tree.h +++ b/flang/include/flang/Parser/dump-parse-tree.h @@ -546,6 +546,7 @@ class ParseTreeDumper { NODE_ENUM(OmpLinearModifier, Type) NODE(parser, OmpLoopDirective) NODE(parser, OmpMapClause) + NODE(parser, OmpMapperIdentifier) NODE_ENUM(OmpMapClause, TypeModifier) NODE_ENUM(OmpMapClause, Type) static std::string GetNodeName(const llvm::omp::Clause &x) { diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h index 10d7840775e88..c7edaa2141969 100644 --- a/flang/include/flang/Parser/parse-tree.h +++ b/flang/include/flang/Parser/parse-tree.h @@ -3703,8 +3703,11 @@ struct OmpLinearClause { std::variant u; }; +WRAPPER_CLASS(OmpMapperIdentifier, std::optional); + // 2.15.5.1 map -> -// MAP ([[map-type-modifier-list [,]] [iterator-modifier [,]] map-type : ] +// MAP ([MAPPER(mapper-identifier)] [[map-type-modifier-list [,]] +// [iterator-modifier [,]] map-type : ] // variable-name-list) // map-type-modifier-list -> map-type-modifier [,] [...] // map-type-modifier -> ALWAYS | CLOSE | PRESENT | OMPX_HOLD @@ -3718,7 +3721,8 @@ struct OmpMapClause { // The checks for satisfying those constraints are deferred to semantics. // In OpenMP 5.2 the non-comma syntax has been deprecated: keep the // information about separator presence to emit a diagnostic if needed. - std::tuple>, + std::tuple>, std::optional>, // unique std::optional>, // unique OmpObjectList, diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp index 3dedd4864bafc..88aa62a62352c 100644 --- a/flang/lib/Lower/OpenMP/Clauses.cpp +++ b/flang/lib/Lower/OpenMP/Clauses.cpp @@ -10,6 +10,7 @@ #include "flang/Common/idioms.h" #include "flang/Evaluate/expression.h" +#include "flang/Optimizer/Builder/Todo.h" #include "flang/Parser/parse-tree.h" #include "flang/Semantics/expression.h" #include "flang/Semantics/symbol.h" @@ -974,6 +975,10 @@ Map make(const parser::OmpClause::Map &inp, std::get>>(inp.v.t); auto &t2 = std::get>>(inp.v.t); auto &t3 = std::get(inp.v.t); + auto &t4 = std::get(inp.v.t); + + if (t4.v) + TODO_NOLOC("OmpMapClause(MAPPER(...))"); // These should have been diagnosed already. assert((!t1 || t1->size() == 1) && "Only one iterator modifier is allowed"); diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp index c2c730edacc02..8259f501dca84 100644 --- a/flang/lib/Parser/openmp-parsers.cpp +++ b/flang/lib/Parser/openmp-parsers.cpp @@ -131,7 +131,6 @@ template struct MotionModifiers { constexpr MotionModifiers(const MotionModifiers &) = default; constexpr MotionModifiers(MotionModifiers &&) = default; - // Parsing of mappers if not implemented yet. using ExpParser = Parser; using IterParser = Parser; using ModParser = ConcatSeparated; @@ -250,21 +249,26 @@ TYPE_PARSER( "TOFROM" >> pure(OmpMapClause::Type::Tofrom))) template -static inline OmpMapClause makeMapClause( +static inline OmpMapClause makeMapClause(OmpMapperIdentifier &&mm, std::tuple>, std::optional>, std::optional>> &&mods, OmpObjectList &&objs) { auto &&[tm, it, ty] = std::move(mods); - return OmpMapClause{std::move(tm), std::move(it), std::move(ty), - std::move(objs), CommasEverywhere}; + return OmpMapClause{std::move(mm), std::move(tm), std::move(it), + std::move(ty), std::move(objs), CommasEverywhere}; } +TYPE_PARSER(construct( + maybe("MAPPER"_tok >> parenthesized(name) / ","_tok))) + TYPE_PARSER(construct( - applyFunction( - makeMapClause, MapModifiers(","_tok), Parser{}) || + applyFunction(makeMapClause, + Parser{}, MapModifiers(","_tok), + Parser{}) || applyFunction(makeMapClause, - MapModifiers(maybe(","_tok)), Parser{}))) + Parser{}, MapModifiers(maybe(","_tok)), + Parser{}))) // [OpenMP 5.0] // 2.19.7.2 defaultmap(implicit-behavior[:variable-category]) diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp index 158d3a1f14e4f..a4de6d23f9f19 100644 --- a/flang/lib/Parser/unparse.cpp +++ b/flang/lib/Parser/unparse.cpp @@ -2095,12 +2095,22 @@ class UnparseVisitor { std::get>>(x.t); auto &iter = std::get>>(x.t); auto &type = std::get>>(x.t); + auto &mapper = std::get(x.t); // For a given list of items, if the item has a value, then walk it. // Print commas between items that have values. // Return 'true' if something did get printed, otherwise 'false'. bool needComma{false}; + if (mapper.v) { + Word("MAPPER("); + Walk(*mapper.v); + Put(")"); + needComma = true; + } if (typeMod) { + if (needComma) { + Put(", "); + } Walk(*typeMod); needComma = true; } diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp index 9b0e204ac4e91..9ebb90587e9e7 100644 --- a/flang/lib/Semantics/resolve-names.cpp +++ b/flang/lib/Semantics/resolve-names.cpp @@ -1471,6 +1471,8 @@ class OmpVisitor : public virtual DeclarationVisitor { bool Pre(const parser::OpenMPDeclareMapperConstruct &); + bool Pre(const parser::OmpMapClause &); + void Post(const parser::OmpBeginLoopDirective &) { messageHandler().set_currStmtSource(std::nullopt); } @@ -1639,6 +1641,14 @@ bool OmpVisitor::Pre(const parser::OpenMPDeclareMapperConstruct &x) { return false; } +bool OmpVisitor::Pre(const parser::OmpMapClause &x) { + const auto &mid{std::get(x.t)}; + if (const auto &mapperName{mid.v}) + mapperName->symbol = + &MakeSymbol(*mapperName, MiscDetails{MiscDetails::Kind::ConstructName}); + return true; +} + // Walk the parse tree and resolve names to symbols. class ResolveNamesVisitor : public virtual ScopeHandler, public ModuleVisitor, diff --git a/flang/test/Lower/OpenMP/Todo/map-mapper.f90 b/flang/test/Lower/OpenMP/Todo/map-mapper.f90 new file mode 100644 index 0000000000000..30831337beff2 --- /dev/null +++ b/flang/test/Lower/OpenMP/Todo/map-mapper.f90 @@ -0,0 +1,11 @@ +! RUN: not %flang_fc1 -emit-fir -fopenmp -fopenmp-version=50 %s 2>&1 | FileCheck %s +program p + integer, parameter :: n = 256 + real(8) :: a(256) + !$omp target map(mapper(xx), from:a) +!CHECK: not yet implemented: OmpMapClause(MAPPER(...)) + do i=1,n + a(i) = 4.2 + end do + !$omp end target +end program p diff --git a/flang/test/Parser/OpenMP/map-modifiers.f90 b/flang/test/Parser/OpenMP/map-modifiers.f90 index 0c95f21c5e6a5..2b4073f2cafcd 100644 --- a/flang/test/Parser/OpenMP/map-modifiers.f90 +++ b/flang/test/Parser/OpenMP/map-modifiers.f90 @@ -316,3 +316,28 @@ subroutine f90(x, y) !PARSE-TREE: | | | | Designator -> DataRef -> Name = 'k' !PARSE-TREE: | | bool = 'true' +subroutine f100(x, y) + integer :: x(10) + integer :: y + integer, parameter :: p = 23 + !$omp target map(mapper(xx), from: x) + x = x + 1 + !$omp end target +end + +!UNPARSE: SUBROUTINE f100 (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: x=x+1_4 +!UNPARSE: !$OMP END TARGET +!UNPARSE: END SUBROUTINE + +!PARSE-TREE: OmpBeginBlockDirective +!PARSE-TREE: | OmpBlockDirective -> llvm::omp::Directive = target +!PARSE-TREE: | OmpClauseList -> OmpClause -> Map -> OmpMapClause +!PARSE-TREE: | | OmpMapperIdentifier -> Name = 'xx' +!PARSE-TREE: | | Type = From +!PARSE-TREE: | | OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'x' + diff --git a/flang/test/Semantics/OpenMP/map-clause-symbols.f90 b/flang/test/Semantics/OpenMP/map-clause-symbols.f90 new file mode 100644 index 0000000000000..8f984fcd2fa7e --- /dev/null +++ b/flang/test/Semantics/OpenMP/map-clause-symbols.f90 @@ -0,0 +1,14 @@ +! RUN: %flang_fc1 -fdebug-dump-symbols -fopenmp -fopenmp-version=50 %s | FileCheck %s +program main +!CHECK-LABEL: MainProgram scope: main + integer, parameter :: n = 256 + real(8) :: a(256) + !$omp target map(mapper(xx), from:a) + do i=1,n + 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 From 042adcfe0ad52eecee2a1ed8db416622cf276ea2 Mon Sep 17 00:00:00 2001 From: Mats Petersson Date: Fri, 15 Nov 2024 14:14:27 +0000 Subject: [PATCH 2/3] Update TODO message to be more user friendly --- flang/lib/Lower/OpenMP/Clauses.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp index 88aa62a62352c..a6f47f2db31ab 100644 --- a/flang/lib/Lower/OpenMP/Clauses.cpp +++ b/flang/lib/Lower/OpenMP/Clauses.cpp @@ -978,7 +978,7 @@ Map make(const parser::OmpClause::Map &inp, auto &t4 = std::get(inp.v.t); if (t4.v) - TODO_NOLOC("OmpMapClause(MAPPER(...))"); + TODO_NOLOC("OmpMapClause(MAPPER(...)): user defined mapper not supported"); // These should have been diagnosed already. assert((!t1 || t1->size() == 1) && "Only one iterator modifier is allowed"); From 7d162afe1e669f8757d5ac23b2fe2b9fafca384c Mon Sep 17 00:00:00 2001 From: Mats Petersson Date: Mon, 18 Nov 2024 18:25:08 +0000 Subject: [PATCH 3/3] Add FindSymbol and update tests --- flang/lib/Semantics/resolve-names.cpp | 25 ++++++++++++++++++--- flang/test/Lower/OpenMP/Todo/map-mapper.f90 | 5 +++++ flang/test/Semantics/OpenMP/map-clause.f90 | 8 +++++++ 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp index 9ebb90587e9e7..7d1df884c5555 100644 --- a/flang/lib/Semantics/resolve-names.cpp +++ b/flang/lib/Semantics/resolve-names.cpp @@ -1643,9 +1643,28 @@ bool OmpVisitor::Pre(const parser::OpenMPDeclareMapperConstruct &x) { bool OmpVisitor::Pre(const parser::OmpMapClause &x) { const auto &mid{std::get(x.t)}; - if (const auto &mapperName{mid.v}) - mapperName->symbol = - &MakeSymbol(*mapperName, MiscDetails{MiscDetails::Kind::ConstructName}); + if (const auto &mapperName{mid.v}) { + if (const auto symbol = FindSymbol(currScope(), *mapperName)) { + // 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()}; + if (!misc || misc->kind() != MiscDetails::Kind::ConstructName) + context().Say(mapperName->source, + "Name '%s' should be a mapper name"_err_en_US, mapperName->source); + else + mapperName->symbol = symbol; + } else { + mapperName->symbol = &MakeSymbol( + *mapperName, 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(mapperName->source, "'%s' not + // declared"_err_en_US, mapperName->source); + } + } return true; } diff --git a/flang/test/Lower/OpenMP/Todo/map-mapper.f90 b/flang/test/Lower/OpenMP/Todo/map-mapper.f90 index 30831337beff2..d83c20db29307 100644 --- a/flang/test/Lower/OpenMP/Todo/map-mapper.f90 +++ b/flang/test/Lower/OpenMP/Todo/map-mapper.f90 @@ -2,6 +2,11 @@ program p integer, parameter :: n = 256 real(8) :: a(256) + !! TODO: Add declare mapper, when it works to lower this construct + !!type t1 + !! integer :: x + !!end type t1 + !!!$omp declare mapper(xx : t1 :: nn) map(nn, nn%x) !$omp target map(mapper(xx), from:a) !CHECK: not yet implemented: OmpMapClause(MAPPER(...)) do i=1,n diff --git a/flang/test/Semantics/OpenMP/map-clause.f90 b/flang/test/Semantics/OpenMP/map-clause.f90 index a7430c3edeb94..efcef2571a04a 100644 --- a/flang/test/Semantics/OpenMP/map-clause.f90 +++ b/flang/test/Semantics/OpenMP/map-clause.f90 @@ -33,3 +33,11 @@ subroutine sb(arr) c = 2 !$omp end target end subroutine + +subroutine sb1 + integer :: xx + integer :: a + !ERROR: Name 'xx' should be a mapper name + !$omp target map(mapper(xx), from:a) + !$omp end target +end subroutine sb1