[OpenMP][Flang] Emit default declare mappers implicitly for derived types #140562
+314 −50
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
llvmbot added flang 
Member
| @llvm/pr-subscribers-github-workflow @llvm/pr-subscribers-flang-openmp Author: Akash Banerjee (TIFitis) ChangesThis patch adds support to emit default declare mappers for implicit mapping of derived types when not supplied by user. This especially helps tackle mapping of allocatables of derived types. Patch is 33.53 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/140562.diff 5 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp index 8dcc8be9be5bf..cf25e91a437b8 100644 --- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp +++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp @@ -1102,23 +1102,30 @@ void ClauseProcessor::processMapObjects( auto getDefaultMapperID = [&](const omp::Object &object, std::string &mapperIdName) { - if (!mlir::isa<mlir::omp::DeclareMapperOp>( - firOpBuilder.getRegion().getParentOp())) { - const semantics::DerivedTypeSpec *typeSpec = nullptr; - - if (object.sym()->owner().IsDerivedType()) - typeSpec = object.sym()->owner().derivedTypeSpec(); - else if (object.sym()->GetType() && - object.sym()->GetType()->category() == - semantics::DeclTypeSpec::TypeDerived) - typeSpec = &object.sym()->GetType()->derivedTypeSpec(); - - if (typeSpec) { - mapperIdName = typeSpec->name().ToString() + ".omp.default.mapper"; - if (auto *sym = converter.getCurrentScope().FindSymbol(mapperIdName)) - mapperIdName = converter.mangleName(mapperIdName, sym->owner()); - } + const semantics::DerivedTypeSpec *typeSpec = nullptr; + + if (object.sym()->GetType() && object.sym()->GetType()->category() == + semantics::DeclTypeSpec::TypeDerived) + typeSpec = &object.sym()->GetType()->derivedTypeSpec(); + else if (object.sym()->owner().IsDerivedType()) + typeSpec = object.sym()->owner().derivedTypeSpec(); + + if (typeSpec) { + mapperIdName = typeSpec->name().ToString() + ".omp.default.mapper"; + if (auto *sym = converter.getCurrentScope().FindSymbol(mapperIdName)) + mapperIdName = converter.mangleName(mapperIdName, sym->owner()); + else + mapperIdName = + converter.mangleName(mapperIdName, *typeSpec->GetScope()); } + + // Make sure we don't return a mapper to self + llvm::StringRef parentOpName; + if (auto declMapOp = mlir::dyn_cast<mlir::omp::DeclareMapperOp>( + firOpBuilder.getRegion().getParentOp())) + parentOpName = declMapOp.getSymName(); + if (mapperIdName == parentOpName) + mapperIdName = ""; }; // Create the mapper symbol from its name, if specified. diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp index cfcba0159db8d..8d5c26a4f2d58 100644 --- a/flang/lib/Lower/OpenMP/OpenMP.cpp +++ b/flang/lib/Lower/OpenMP/OpenMP.cpp @@ -2348,6 +2348,124 @@ genSingleOp(lower::AbstractConverter &converter, lower::SymMap &symTable, queue, item, clauseOps); } +static mlir::FlatSymbolRefAttr +genImplicitDefaultDeclareMapper(lower::AbstractConverter &converter, + mlir::Location loc, fir::RecordType recordType, + llvm::StringRef mapperNameStr) { + fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder(); + lower::StatementContext stmtCtx; + + // Save current insertion point before moving to the module scope to create + // the DeclareMapperOp + mlir::OpBuilder::InsertionGuard guard(firOpBuilder); + + firOpBuilder.setInsertionPointToStart(converter.getModuleOp().getBody()); + auto declMapperOp = firOpBuilder.create<mlir::omp::DeclareMapperOp>( + loc, mapperNameStr, recordType); + auto ®ion = declMapperOp.getRegion(); + firOpBuilder.createBlock(®ion); + auto mapperArg = region.addArgument(firOpBuilder.getRefType(recordType), loc); + + auto declareOp = + firOpBuilder.create<hlfir::DeclareOp>(loc, mapperArg, /*uniq_name=*/""); + + const auto genBoundsOps = [&](mlir::Value mapVal, + llvm::SmallVectorImpl<mlir::Value> &bounds) { + fir::ExtendedValue extVal = + hlfir::translateToExtendedValue(mapVal.getLoc(), firOpBuilder, + hlfir::Entity{mapVal}, + /*contiguousHint=*/true) + .first; + fir::factory::AddrAndBoundsInfo info = fir::factory::getDataOperandBaseAddr( + firOpBuilder, mapVal, /*isOptional=*/false, mapVal.getLoc()); + bounds = fir::factory::genImplicitBoundsOps<mlir::omp::MapBoundsOp, + mlir::omp::MapBoundsType>( + firOpBuilder, info, extVal, + /*dataExvIsAssumedSize=*/false, mapVal.getLoc()); + }; + + // Return a reference to the contents of a derived type with one field. + // Also return the field type. + const auto getFieldRef = + [&](mlir::Value rec, + unsigned index) -> std::tuple<mlir::Value, mlir::Type> { + auto recType = mlir::dyn_cast<fir::RecordType>( + fir::unwrapPassByRefType(rec.getType())); + auto [fieldName, fieldTy] = recType.getTypeList()[index]; + mlir::Value field = firOpBuilder.create<fir::FieldIndexOp>( + loc, fir::FieldType::get(recType.getContext()), fieldName, recType, + fir::getTypeParams(rec)); + return {firOpBuilder.create<fir::CoordinateOp>( + loc, firOpBuilder.getRefType(fieldTy), rec, field), + fieldTy}; + }; + + mlir::omp::DeclareMapperInfoOperands clauseOps; + llvm::SmallVector<llvm::SmallVector<int64_t>> memberPlacementIndices; + llvm::SmallVector<mlir::Value> memberMapOps; + + llvm::omp::OpenMPOffloadMappingFlags mapFlag = + llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO | + llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM | + llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_IMPLICIT; + mlir::omp::VariableCaptureKind captureKind = + mlir::omp::VariableCaptureKind::ByRef; + int64_t index = 0; + + // Populate the declareMapper region with the map information. + for (const auto &[memberName, memberType] : + mlir::dyn_cast<fir::RecordType>(recordType).getTypeList()) { + auto [ref, type] = getFieldRef(declareOp.getBase(), index); + mlir::FlatSymbolRefAttr mapperId; + if (auto recType = mlir::dyn_cast<fir::RecordType>(memberType)) { + std::string mapperIdName = + recType.getName().str() + ".omp.default.mapper"; + if (auto *sym = converter.getCurrentScope().FindSymbol(mapperIdName)) + mapperIdName = converter.mangleName(mapperIdName, sym->owner()); + else if (auto *sym = converter.getCurrentScope().FindSymbol(memberName)) + mapperIdName = converter.mangleName(mapperIdName, sym->owner()); + + if (converter.getModuleOp().lookupSymbol(mapperIdName)) + mapperId = mlir::FlatSymbolRefAttr::get(&converter.getMLIRContext(), + mapperIdName); + else + mapperId = genImplicitDefaultDeclareMapper(converter, loc, recType, + mapperIdName); + } + + llvm::SmallVector<mlir::Value> bounds; + genBoundsOps(ref, bounds); + mlir::Value mapOp = createMapInfoOp( + firOpBuilder, loc, ref, /*varPtrPtr=*/mlir::Value{}, "", bounds, + /*members=*/{}, + /*membersIndex=*/mlir::ArrayAttr{}, + static_cast< + std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>( + mapFlag), + captureKind, ref.getType(), /*partialMap=*/false, mapperId); + memberMapOps.emplace_back(mapOp); + memberPlacementIndices.emplace_back(llvm::SmallVector<int64_t>{index++}); + } + + llvm::SmallVector<mlir::Value> bounds; + genBoundsOps(declareOp.getOriginalBase(), bounds); + mlir::omp::MapInfoOp mapOp = createMapInfoOp( + firOpBuilder, loc, declareOp.getOriginalBase(), + /*varPtrPtr=*/mlir::Value(), /*name=*/"", bounds, memberMapOps, + firOpBuilder.create2DI64ArrayAttr(memberPlacementIndices), + static_cast<std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>( + mapFlag), + captureKind, declareOp.getType(0), + /*partialMap=*/true); + + clauseOps.mapVars.emplace_back(mapOp); + + firOpBuilder.create<mlir::omp::DeclareMapperInfoOp>(loc, clauseOps.mapVars); + // declMapperOp->dumpPretty(); + return mlir::FlatSymbolRefAttr::get(&converter.getMLIRContext(), + mapperNameStr); +} + static mlir::omp::TargetOp genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable, lower::StatementContext &stmtCtx, @@ -2420,15 +2538,26 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable, name << sym.name().ToString(); mlir::FlatSymbolRefAttr mapperId; - if (sym.GetType()->category() == semantics::DeclTypeSpec::TypeDerived) { + if (sym.GetType()->category() == semantics::DeclTypeSpec::TypeDerived && + defaultMaps.empty()) { auto &typeSpec = sym.GetType()->derivedTypeSpec(); std::string mapperIdName = typeSpec.name().ToString() + ".omp.default.mapper"; if (auto *sym = converter.getCurrentScope().FindSymbol(mapperIdName)) mapperIdName = converter.mangleName(mapperIdName, sym->owner()); + else + mapperIdName = + converter.mangleName(mapperIdName, *typeSpec.GetScope()); + if (converter.getModuleOp().lookupSymbol(mapperIdName)) mapperId = mlir::FlatSymbolRefAttr::get(&converter.getMLIRContext(), mapperIdName); + else + mapperId = genImplicitDefaultDeclareMapper( + converter, loc, + mlir::cast<fir::RecordType>( + converter.genType(sym.GetType()->derivedTypeSpec())), + mapperIdName); } fir::factory::AddrAndBoundsInfo info = @@ -4039,6 +4168,7 @@ genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable, ClauseProcessor cp(converter, semaCtx, clauses); cp.processMap(loc, stmtCtx, clauseOps); firOpBuilder.create<mlir::omp::DeclareMapperInfoOp>(loc, clauseOps.mapVars); + // declMapperOp->dumpPretty(); } static void diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp index 86095d708f7e5..1ef6239e51fa8 100644 --- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp +++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp @@ -377,7 +377,7 @@ class MapInfoFinalizationPass getDescriptorMapType(mapType, target)), op.getMapCaptureTypeAttr(), /*varPtrPtr=*/mlir::Value{}, newMembers, newMembersAttr, /*bounds=*/mlir::SmallVector<mlir::Value>{}, - /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), + op.getMapperIdAttr(), op.getNameAttr(), /*partial_map=*/builder.getBoolAttr(false)); op.replaceAllUsesWith(newDescParentMapOp.getResult()); op->erase(); diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp index 322562b06b87f..318af99a57848 100644 --- a/flang/lib/Semantics/resolve-names.cpp +++ b/flang/lib/Semantics/resolve-names.cpp @@ -2163,7 +2163,7 @@ void AttrsVisitor::SetBindNameOn(Symbol &symbol) { } symbol.SetBindName(std::move(*label)); if (!oldBindName.empty()) { - if (const std::string * newBindName{symbol.GetBindName()}) { + if (const std::string *newBindName{symbol.GetBindName()}) { if (oldBindName != *newBindName) { Say(symbol.name(), "The entity '%s' has multiple BIND names ('%s' and '%s')"_err_en_US, @@ -2285,7 +2285,7 @@ void DeclTypeSpecVisitor::Post(const parser::TypeSpec &typeSpec) { // expression semantics if the DeclTypeSpec is a valid TypeSpec. // The grammar ensures that it's an intrinsic or derived type spec, // not TYPE(*) or CLASS(*) or CLASS(T). - if (const DeclTypeSpec * spec{state_.declTypeSpec}) { + if (const DeclTypeSpec *spec{state_.declTypeSpec}) { switch (spec->category()) { case DeclTypeSpec::Numeric: case DeclTypeSpec::Logical: @@ -2293,7 +2293,7 @@ void DeclTypeSpecVisitor::Post(const parser::TypeSpec &typeSpec) { typeSpec.declTypeSpec = spec; break; case DeclTypeSpec::TypeDerived: - if (const DerivedTypeSpec * derived{spec->AsDerived()}) { + if (const DerivedTypeSpec *derived{spec->AsDerived()}) { CheckForAbstractType(derived->typeSymbol()); // C703 typeSpec.declTypeSpec = spec; } @@ -2802,8 +2802,8 @@ Symbol &ScopeHandler::MakeSymbol(const parser::Name &name, Attrs attrs) { Symbol &ScopeHandler::MakeHostAssocSymbol( const parser::Name &name, const Symbol &hostSymbol) { Symbol &symbol{*NonDerivedTypeScope() - .try_emplace(name.source, HostAssocDetails{hostSymbol}) - .first->second}; + .try_emplace(name.source, HostAssocDetails{hostSymbol}) + .first->second}; name.symbol = &symbol; symbol.attrs() = hostSymbol.attrs(); // TODO: except PRIVATE, PUBLIC? // These attributes can be redundantly reapplied without error @@ -2891,7 +2891,7 @@ void ScopeHandler::ApplyImplicitRules( if (context().HasError(symbol) || !NeedsType(symbol)) { return; } - if (const DeclTypeSpec * type{GetImplicitType(symbol)}) { + if (const DeclTypeSpec *type{GetImplicitType(symbol)}) { if (!skipImplicitTyping_) { symbol.set(Symbol::Flag::Implicit); symbol.SetType(*type); @@ -2991,7 +2991,7 @@ const DeclTypeSpec *ScopeHandler::GetImplicitType( const auto *type{implicitRulesMap_->at(scope).GetType( symbol.name(), respectImplicitNoneType)}; if (type) { - if (const DerivedTypeSpec * derived{type->AsDerived()}) { + if (const DerivedTypeSpec *derived{type->AsDerived()}) { // Resolve any forward-referenced derived type; a quick no-op else. auto &instantiatable{*const_cast<DerivedTypeSpec *>(derived)}; instantiatable.Instantiate(currScope()); @@ -3706,10 +3706,10 @@ void ModuleVisitor::DoAddUse(SourceName location, SourceName localName, } else if (IsPointer(p1) || IsPointer(p2)) { return false; } else if (const auto *subp{p1.detailsIf<SubprogramDetails>()}; - subp && !subp->isInterface()) { + subp && !subp->isInterface()) { return false; // defined in module, not an external } else if (const auto *subp{p2.detailsIf<SubprogramDetails>()}; - subp && !subp->isInterface()) { + subp && !subp->isInterface()) { return false; // defined in module, not an external } else { // Both are external interfaces, perhaps to the same procedure @@ -3969,7 +3969,7 @@ Scope *ModuleVisitor::FindModule(const parser::Name &name, if (scope) { if (DoesScopeContain(scope, currScope())) { // 14.2.2(1) std::optional<SourceName> submoduleName; - if (const Scope * container{FindModuleOrSubmoduleContaining(currScope())}; + if (const Scope *container{FindModuleOrSubmoduleContaining(currScope())}; container && container->IsSubmodule()) { submoduleName = container->GetName(); } @@ -4074,7 +4074,7 @@ bool InterfaceVisitor::isAbstract() const { void InterfaceVisitor::AddSpecificProcs( const std::list<parser::Name> &names, ProcedureKind kind) { - if (Symbol * symbol{GetGenericInfo().symbol}; + if (Symbol *symbol{GetGenericInfo().symbol}; symbol && symbol->has<GenericDetails>()) { for (const auto &name : names) { specificsForGenericProcs_.emplace(symbol, std::make_pair(&name, kind)); @@ -4174,7 +4174,7 @@ void GenericHandler::DeclaredPossibleSpecificProc(Symbol &proc) { } void InterfaceVisitor::ResolveNewSpecifics() { - if (Symbol * generic{genericInfo_.top().symbol}; + if (Symbol *generic{genericInfo_.top().symbol}; generic && generic->has<GenericDetails>()) { ResolveSpecificsInGeneric(*generic, false); } @@ -4259,7 +4259,7 @@ bool SubprogramVisitor::HandleStmtFunction(const parser::StmtFunctionStmt &x) { name.source); MakeSymbol(name, Attrs{}, UnknownDetails{}); } else if (auto *entity{ultimate.detailsIf<EntityDetails>()}; - entity && !ultimate.has<ProcEntityDetails>()) { + entity && !ultimate.has<ProcEntityDetails>()) { resultType = entity->type(); ultimate.details() = UnknownDetails{}; // will be replaced below } else { @@ -4315,7 +4315,7 @@ bool SubprogramVisitor::Pre(const parser::Suffix &suffix) { } else { Message &msg{Say(*suffix.resultName, "RESULT(%s) may appear only in a function"_err_en_US)}; - if (const Symbol * subprogram{InclusiveScope().symbol()}) { + if (const Symbol *subprogram{InclusiveScope().symbol()}) { msg.Attach(subprogram->name(), "Containing subprogram"_en_US); } } @@ -4833,7 +4833,7 @@ Symbol *ScopeHandler::FindSeparateModuleProcedureInterface( symbol = generic->specific(); } } - if (const Symbol * defnIface{FindSeparateModuleSubprogramInterface(symbol)}) { + if (const Symbol *defnIface{FindSeparateModuleSubprogramInterface(symbol)}) { // Error recovery in case of multiple definitions symbol = const_cast<Symbol *>(defnIface); } @@ -4970,8 +4970,8 @@ bool SubprogramVisitor::HandlePreviousCalls( return generic->specific() && HandlePreviousCalls(name, *generic->specific(), subpFlag); } else if (const auto *proc{symbol.detailsIf<ProcEntityDetails>()}; proc && - !proc->isDummy() && - !symbol.attrs().HasAny(Attrs{Attr::INTRINSIC, Attr::POINTER})) { + !proc->isDummy() && + !symbol.attrs().HasAny(Attrs{Attr::INTRINSIC, Attr::POINTER})) { // There's a symbol created for previous calls to this subprogram or // ENTRY's name. We have to replace that symbol in situ to avoid the // obligation to rewrite symbol pointers in the parse tree. @@ -5012,7 +5012,7 @@ void SubprogramVisitor::CheckExtantProc( if (auto *prev{FindSymbol(name)}) { if (IsDummy(*prev)) { } else if (auto *entity{prev->detailsIf<EntityDetails>()}; - IsPointer(*prev) && entity && !entity->type()) { + IsPointer(*prev) && entity && !entity->type()) { // POINTER attribute set before interface } else if (inInterfaceBlock() && currScope() != prev->owner()) { // Procedures in an INTERFACE block do not resolve to symbols @@ -5068,7 +5068,7 @@ Symbol &SubprogramVisitor::PushSubprogramScope(const parser::Name &name, } set_inheritFromParent(false); // interfaces don't inherit, even if MODULE } - if (Symbol * found{FindSymbol(name)}; + if (Symbol *found{FindSymbol(name)}; found && found->has<HostAssocDetails>()) { found->set(subpFlag); // PushScope() created symbol } @@ -5910,9 +5910,9 @@ void DeclarationVisitor::Post(const parser::VectorTypeSpec &x) { vectorDerivedType.CookParameters(GetFoldingContext()); } - if (const DeclTypeSpec * - extant{ppcBuiltinTypesScope->FindInstantiatedDerivedType( - vectorDerivedType, DeclTypeSpec::Category::TypeDerived)}) { + if (const DeclTypeSpec *extant{ + ppcBuiltinTypesScope->FindInstantiatedDerivedType( + vectorDerivedType, DeclTypeSpec::Category::TypeDerived)}) { // This derived type and parameter expressions (if any) are already present // in the __ppc_intrinsics scope. SetDeclTypeSpec(*extant); @@ -5934,7 +5934,7 @@ bool DeclarationVisitor::Pre(const parser::DeclarationTypeSpec::Type &) { void DeclarationVisitor::Post(const parser::DeclarationTypeSpec::Type &type) { const parser::Name &derivedName{std::get<parser::Name>(type.derived.t)}; - if (const Symbol * derivedSymbol{derivedName.symbol}) { + if (const Symbol *derivedSymbol{derivedName.symbol}) { CheckForAbstractType(*derivedSymbol); // C706 } } @@ -6003,8 +6003,8 @@ void DeclarationVisitor::Post(const parser::DerivedTypeSpec &x) { if (!spec->MightBeParameterized()) { spec->EvaluateParameters(context()); } - if (const DeclTypeSpec * - extant{currScope().FindInstantiatedDerivedType(*spec, category)}) { + if (const DeclTypeSpec *extant{ + currScope().FindInstantiatedDerivedType(*spec, category)}) { // This derived type and parameter expressions (if any) are already present // in this scope. SetDeclTypeSpec(*extant); @@ -6035,8 +6035,7 @@ void DeclarationVisitor::Post(const parser::DeclarationTypeSpec::Record &rec) { if (auto spec{ResolveDerivedType(typeName)}) { spec->CookParameters(GetFoldingContext()); ... [truncated] |
Member
| @llvm/pr-subscribers-flang-fir-hlfir Author: Akash Banerjee (TIFitis) ChangesThis patch adds support to emit default declare mappers for implicit mapping of derived types when not supplied by user. This especially helps tackle mapping of allocatables of derived types. Patch is 33.53 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/140562.diff 5 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp index 8dcc8be9be5bf..cf25e91a437b8 100644 --- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp +++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp @@ -1102,23 +1102,30 @@ void ClauseProcessor::processMapObjects( auto getDefaultMapperID = [&](const omp::Object &object, std::string &mapperIdName) { - if (!mlir::isa<mlir::omp::DeclareMapperOp>( - firOpBuilder.getRegion().getParentOp())) { - const semantics::DerivedTypeSpec *typeSpec = nullptr; - - if (object.sym()->owner().IsDerivedType()) - typeSpec = object.sym()->owner().derivedTypeSpec(); - else if (object.sym()->GetType() && - object.sym()->GetType()->category() == - semantics::DeclTypeSpec::TypeDerived) - typeSpec = &object.sym()->GetType()->derivedTypeSpec(); - - if (typeSpec) { - mapperIdName = typeSpec->name().ToString() + ".omp.default.mapper"; - if (auto *sym = converter.getCurrentScope().FindSymbol(mapperIdName)) - mapperIdName = converter.mangleName(mapperIdName, sym->owner()); - } + const semantics::DerivedTypeSpec *typeSpec = nullptr; + + if (object.sym()->GetType() && object.sym()->GetType()->category() == + semantics::DeclTypeSpec::TypeDerived) + typeSpec = &object.sym()->GetType()->derivedTypeSpec(); + else if (object.sym()->owner().IsDerivedType()) + typeSpec = object.sym()->owner().derivedTypeSpec(); + + if (typeSpec) { + mapperIdName = typeSpec->name().ToString() + ".omp.default.mapper"; + if (auto *sym = converter.getCurrentScope().FindSymbol(mapperIdName)) + mapperIdName = converter.mangleName(mapperIdName, sym->owner()); + else + mapperIdName = + converter.mangleName(mapperIdName, *typeSpec->GetScope()); } + + // Make sure we don't return a mapper to self + llvm::StringRef parentOpName; + if (auto declMapOp = mlir::dyn_cast<mlir::omp::DeclareMapperOp>( + firOpBuilder.getRegion().getParentOp())) + parentOpName = declMapOp.getSymName(); + if (mapperIdName == parentOpName) + mapperIdName = ""; }; // Create the mapper symbol from its name, if specified. diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp index cfcba0159db8d..8d5c26a4f2d58 100644 --- a/flang/lib/Lower/OpenMP/OpenMP.cpp +++ b/flang/lib/Lower/OpenMP/OpenMP.cpp @@ -2348,6 +2348,124 @@ genSingleOp(lower::AbstractConverter &converter, lower::SymMap &symTable, queue, item, clauseOps); } +static mlir::FlatSymbolRefAttr +genImplicitDefaultDeclareMapper(lower::AbstractConverter &converter, + mlir::Location loc, fir::RecordType recordType, + llvm::StringRef mapperNameStr) { + fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder(); + lower::StatementContext stmtCtx; + + // Save current insertion point before moving to the module scope to create + // the DeclareMapperOp + mlir::OpBuilder::InsertionGuard guard(firOpBuilder); + + firOpBuilder.setInsertionPointToStart(converter.getModuleOp().getBody()); + auto declMapperOp = firOpBuilder.create<mlir::omp::DeclareMapperOp>( + loc, mapperNameStr, recordType); + auto ®ion = declMapperOp.getRegion(); + firOpBuilder.createBlock(®ion); + auto mapperArg = region.addArgument(firOpBuilder.getRefType(recordType), loc); + + auto declareOp = + firOpBuilder.create<hlfir::DeclareOp>(loc, mapperArg, /*uniq_name=*/""); + + const auto genBoundsOps = [&](mlir::Value mapVal, + llvm::SmallVectorImpl<mlir::Value> &bounds) { + fir::ExtendedValue extVal = + hlfir::translateToExtendedValue(mapVal.getLoc(), firOpBuilder, + hlfir::Entity{mapVal}, + /*contiguousHint=*/true) + .first; + fir::factory::AddrAndBoundsInfo info = fir::factory::getDataOperandBaseAddr( + firOpBuilder, mapVal, /*isOptional=*/false, mapVal.getLoc()); + bounds = fir::factory::genImplicitBoundsOps<mlir::omp::MapBoundsOp, + mlir::omp::MapBoundsType>( + firOpBuilder, info, extVal, + /*dataExvIsAssumedSize=*/false, mapVal.getLoc()); + }; + + // Return a reference to the contents of a derived type with one field. + // Also return the field type. + const auto getFieldRef = + [&](mlir::Value rec, + unsigned index) -> std::tuple<mlir::Value, mlir::Type> { + auto recType = mlir::dyn_cast<fir::RecordType>( + fir::unwrapPassByRefType(rec.getType())); + auto [fieldName, fieldTy] = recType.getTypeList()[index]; + mlir::Value field = firOpBuilder.create<fir::FieldIndexOp>( + loc, fir::FieldType::get(recType.getContext()), fieldName, recType, + fir::getTypeParams(rec)); + return {firOpBuilder.create<fir::CoordinateOp>( + loc, firOpBuilder.getRefType(fieldTy), rec, field), + fieldTy}; + }; + + mlir::omp::DeclareMapperInfoOperands clauseOps; + llvm::SmallVector<llvm::SmallVector<int64_t>> memberPlacementIndices; + llvm::SmallVector<mlir::Value> memberMapOps; + + llvm::omp::OpenMPOffloadMappingFlags mapFlag = + llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO | + llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM | + llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_IMPLICIT; + mlir::omp::VariableCaptureKind captureKind = + mlir::omp::VariableCaptureKind::ByRef; + int64_t index = 0; + + // Populate the declareMapper region with the map information. + for (const auto &[memberName, memberType] : + mlir::dyn_cast<fir::RecordType>(recordType).getTypeList()) { + auto [ref, type] = getFieldRef(declareOp.getBase(), index); + mlir::FlatSymbolRefAttr mapperId; + if (auto recType = mlir::dyn_cast<fir::RecordType>(memberType)) { + std::string mapperIdName = + recType.getName().str() + ".omp.default.mapper"; + if (auto *sym = converter.getCurrentScope().FindSymbol(mapperIdName)) + mapperIdName = converter.mangleName(mapperIdName, sym->owner()); + else if (auto *sym = converter.getCurrentScope().FindSymbol(memberName)) + mapperIdName = converter.mangleName(mapperIdName, sym->owner()); + + if (converter.getModuleOp().lookupSymbol(mapperIdName)) + mapperId = mlir::FlatSymbolRefAttr::get(&converter.getMLIRContext(), + mapperIdName); + else + mapperId = genImplicitDefaultDeclareMapper(converter, loc, recType, + mapperIdName); + } + + llvm::SmallVector<mlir::Value> bounds; + genBoundsOps(ref, bounds); + mlir::Value mapOp = createMapInfoOp( + firOpBuilder, loc, ref, /*varPtrPtr=*/mlir::Value{}, "", bounds, + /*members=*/{}, + /*membersIndex=*/mlir::ArrayAttr{}, + static_cast< + std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>( + mapFlag), + captureKind, ref.getType(), /*partialMap=*/false, mapperId); + memberMapOps.emplace_back(mapOp); + memberPlacementIndices.emplace_back(llvm::SmallVector<int64_t>{index++}); + } + + llvm::SmallVector<mlir::Value> bounds; + genBoundsOps(declareOp.getOriginalBase(), bounds); + mlir::omp::MapInfoOp mapOp = createMapInfoOp( + firOpBuilder, loc, declareOp.getOriginalBase(), + /*varPtrPtr=*/mlir::Value(), /*name=*/"", bounds, memberMapOps, + firOpBuilder.create2DI64ArrayAttr(memberPlacementIndices), + static_cast<std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>( + mapFlag), + captureKind, declareOp.getType(0), + /*partialMap=*/true); + + clauseOps.mapVars.emplace_back(mapOp); + + firOpBuilder.create<mlir::omp::DeclareMapperInfoOp>(loc, clauseOps.mapVars); + // declMapperOp->dumpPretty(); + return mlir::FlatSymbolRefAttr::get(&converter.getMLIRContext(), + mapperNameStr); +} + static mlir::omp::TargetOp genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable, lower::StatementContext &stmtCtx, @@ -2420,15 +2538,26 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable, name << sym.name().ToString(); mlir::FlatSymbolRefAttr mapperId; - if (sym.GetType()->category() == semantics::DeclTypeSpec::TypeDerived) { + if (sym.GetType()->category() == semantics::DeclTypeSpec::TypeDerived && + defaultMaps.empty()) { auto &typeSpec = sym.GetType()->derivedTypeSpec(); std::string mapperIdName = typeSpec.name().ToString() + ".omp.default.mapper"; if (auto *sym = converter.getCurrentScope().FindSymbol(mapperIdName)) mapperIdName = converter.mangleName(mapperIdName, sym->owner()); + else + mapperIdName = + converter.mangleName(mapperIdName, *typeSpec.GetScope()); + if (converter.getModuleOp().lookupSymbol(mapperIdName)) mapperId = mlir::FlatSymbolRefAttr::get(&converter.getMLIRContext(), mapperIdName); + else + mapperId = genImplicitDefaultDeclareMapper( + converter, loc, + mlir::cast<fir::RecordType>( + converter.genType(sym.GetType()->derivedTypeSpec())), + mapperIdName); } fir::factory::AddrAndBoundsInfo info = @@ -4039,6 +4168,7 @@ genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable, ClauseProcessor cp(converter, semaCtx, clauses); cp.processMap(loc, stmtCtx, clauseOps); firOpBuilder.create<mlir::omp::DeclareMapperInfoOp>(loc, clauseOps.mapVars); + // declMapperOp->dumpPretty(); } static void diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp index 86095d708f7e5..1ef6239e51fa8 100644 --- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp +++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp @@ -377,7 +377,7 @@ class MapInfoFinalizationPass getDescriptorMapType(mapType, target)), op.getMapCaptureTypeAttr(), /*varPtrPtr=*/mlir::Value{}, newMembers, newMembersAttr, /*bounds=*/mlir::SmallVector<mlir::Value>{}, - /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), + op.getMapperIdAttr(), op.getNameAttr(), /*partial_map=*/builder.getBoolAttr(false)); op.replaceAllUsesWith(newDescParentMapOp.getResult()); op->erase(); diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp index 322562b06b87f..318af99a57848 100644 --- a/flang/lib/Semantics/resolve-names.cpp +++ b/flang/lib/Semantics/resolve-names.cpp @@ -2163,7 +2163,7 @@ void AttrsVisitor::SetBindNameOn(Symbol &symbol) { } symbol.SetBindName(std::move(*label)); if (!oldBindName.empty()) { - if (const std::string * newBindName{symbol.GetBindName()}) { + if (const std::string *newBindName{symbol.GetBindName()}) { if (oldBindName != *newBindName) { Say(symbol.name(), "The entity '%s' has multiple BIND names ('%s' and '%s')"_err_en_US, @@ -2285,7 +2285,7 @@ void DeclTypeSpecVisitor::Post(const parser::TypeSpec &typeSpec) { // expression semantics if the DeclTypeSpec is a valid TypeSpec. // The grammar ensures that it's an intrinsic or derived type spec, // not TYPE(*) or CLASS(*) or CLASS(T). - if (const DeclTypeSpec * spec{state_.declTypeSpec}) { + if (const DeclTypeSpec *spec{state_.declTypeSpec}) { switch (spec->category()) { case DeclTypeSpec::Numeric: case DeclTypeSpec::Logical: @@ -2293,7 +2293,7 @@ void DeclTypeSpecVisitor::Post(const parser::TypeSpec &typeSpec) { typeSpec.declTypeSpec = spec; break; case DeclTypeSpec::TypeDerived: - if (const DerivedTypeSpec * derived{spec->AsDerived()}) { + if (const DerivedTypeSpec *derived{spec->AsDerived()}) { CheckForAbstractType(derived->typeSymbol()); // C703 typeSpec.declTypeSpec = spec; } @@ -2802,8 +2802,8 @@ Symbol &ScopeHandler::MakeSymbol(const parser::Name &name, Attrs attrs) { Symbol &ScopeHandler::MakeHostAssocSymbol( const parser::Name &name, const Symbol &hostSymbol) { Symbol &symbol{*NonDerivedTypeScope() - .try_emplace(name.source, HostAssocDetails{hostSymbol}) - .first->second}; + .try_emplace(name.source, HostAssocDetails{hostSymbol}) + .first->second}; name.symbol = &symbol; symbol.attrs() = hostSymbol.attrs(); // TODO: except PRIVATE, PUBLIC? // These attributes can be redundantly reapplied without error @@ -2891,7 +2891,7 @@ void ScopeHandler::ApplyImplicitRules( if (context().HasError(symbol) || !NeedsType(symbol)) { return; } - if (const DeclTypeSpec * type{GetImplicitType(symbol)}) { + if (const DeclTypeSpec *type{GetImplicitType(symbol)}) { if (!skipImplicitTyping_) { symbol.set(Symbol::Flag::Implicit); symbol.SetType(*type); @@ -2991,7 +2991,7 @@ const DeclTypeSpec *ScopeHandler::GetImplicitType( const auto *type{implicitRulesMap_->at(scope).GetType( symbol.name(), respectImplicitNoneType)}; if (type) { - if (const DerivedTypeSpec * derived{type->AsDerived()}) { + if (const DerivedTypeSpec *derived{type->AsDerived()}) { // Resolve any forward-referenced derived type; a quick no-op else. auto &instantiatable{*const_cast<DerivedTypeSpec *>(derived)}; instantiatable.Instantiate(currScope()); @@ -3706,10 +3706,10 @@ void ModuleVisitor::DoAddUse(SourceName location, SourceName localName, } else if (IsPointer(p1) || IsPointer(p2)) { return false; } else if (const auto *subp{p1.detailsIf<SubprogramDetails>()}; - subp && !subp->isInterface()) { + subp && !subp->isInterface()) { return false; // defined in module, not an external } else if (const auto *subp{p2.detailsIf<SubprogramDetails>()}; - subp && !subp->isInterface()) { + subp && !subp->isInterface()) { return false; // defined in module, not an external } else { // Both are external interfaces, perhaps to the same procedure @@ -3969,7 +3969,7 @@ Scope *ModuleVisitor::FindModule(const parser::Name &name, if (scope) { if (DoesScopeContain(scope, currScope())) { // 14.2.2(1) std::optional<SourceName> submoduleName; - if (const Scope * container{FindModuleOrSubmoduleContaining(currScope())}; + if (const Scope *container{FindModuleOrSubmoduleContaining(currScope())}; container && container->IsSubmodule()) { submoduleName = container->GetName(); } @@ -4074,7 +4074,7 @@ bool InterfaceVisitor::isAbstract() const { void InterfaceVisitor::AddSpecificProcs( const std::list<parser::Name> &names, ProcedureKind kind) { - if (Symbol * symbol{GetGenericInfo().symbol}; + if (Symbol *symbol{GetGenericInfo().symbol}; symbol && symbol->has<GenericDetails>()) { for (const auto &name : names) { specificsForGenericProcs_.emplace(symbol, std::make_pair(&name, kind)); @@ -4174,7 +4174,7 @@ void GenericHandler::DeclaredPossibleSpecificProc(Symbol &proc) { } void InterfaceVisitor::ResolveNewSpecifics() { - if (Symbol * generic{genericInfo_.top().symbol}; + if (Symbol *generic{genericInfo_.top().symbol}; generic && generic->has<GenericDetails>()) { ResolveSpecificsInGeneric(*generic, false); } @@ -4259,7 +4259,7 @@ bool SubprogramVisitor::HandleStmtFunction(const parser::StmtFunctionStmt &x) { name.source); MakeSymbol(name, Attrs{}, UnknownDetails{}); } else if (auto *entity{ultimate.detailsIf<EntityDetails>()}; - entity && !ultimate.has<ProcEntityDetails>()) { + entity && !ultimate.has<ProcEntityDetails>()) { resultType = entity->type(); ultimate.details() = UnknownDetails{}; // will be replaced below } else { @@ -4315,7 +4315,7 @@ bool SubprogramVisitor::Pre(const parser::Suffix &suffix) { } else { Message &msg{Say(*suffix.resultName, "RESULT(%s) may appear only in a function"_err_en_US)}; - if (const Symbol * subprogram{InclusiveScope().symbol()}) { + if (const Symbol *subprogram{InclusiveScope().symbol()}) { msg.Attach(subprogram->name(), "Containing subprogram"_en_US); } } @@ -4833,7 +4833,7 @@ Symbol *ScopeHandler::FindSeparateModuleProcedureInterface( symbol = generic->specific(); } } - if (const Symbol * defnIface{FindSeparateModuleSubprogramInterface(symbol)}) { + if (const Symbol *defnIface{FindSeparateModuleSubprogramInterface(symbol)}) { // Error recovery in case of multiple definitions symbol = const_cast<Symbol *>(defnIface); } @@ -4970,8 +4970,8 @@ bool SubprogramVisitor::HandlePreviousCalls( return generic->specific() && HandlePreviousCalls(name, *generic->specific(), subpFlag); } else if (const auto *proc{symbol.detailsIf<ProcEntityDetails>()}; proc && - !proc->isDummy() && - !symbol.attrs().HasAny(Attrs{Attr::INTRINSIC, Attr::POINTER})) { + !proc->isDummy() && + !symbol.attrs().HasAny(Attrs{Attr::INTRINSIC, Attr::POINTER})) { // There's a symbol created for previous calls to this subprogram or // ENTRY's name. We have to replace that symbol in situ to avoid the // obligation to rewrite symbol pointers in the parse tree. @@ -5012,7 +5012,7 @@ void SubprogramVisitor::CheckExtantProc( if (auto *prev{FindSymbol(name)}) { if (IsDummy(*prev)) { } else if (auto *entity{prev->detailsIf<EntityDetails>()}; - IsPointer(*prev) && entity && !entity->type()) { + IsPointer(*prev) && entity && !entity->type()) { // POINTER attribute set before interface } else if (inInterfaceBlock() && currScope() != prev->owner()) { // Procedures in an INTERFACE block do not resolve to symbols @@ -5068,7 +5068,7 @@ Symbol &SubprogramVisitor::PushSubprogramScope(const parser::Name &name, } set_inheritFromParent(false); // interfaces don't inherit, even if MODULE } - if (Symbol * found{FindSymbol(name)}; + if (Symbol *found{FindSymbol(name)}; found && found->has<HostAssocDetails>()) { found->set(subpFlag); // PushScope() created symbol } @@ -5910,9 +5910,9 @@ void DeclarationVisitor::Post(const parser::VectorTypeSpec &x) { vectorDerivedType.CookParameters(GetFoldingContext()); } - if (const DeclTypeSpec * - extant{ppcBuiltinTypesScope->FindInstantiatedDerivedType( - vectorDerivedType, DeclTypeSpec::Category::TypeDerived)}) { + if (const DeclTypeSpec *extant{ + ppcBuiltinTypesScope->FindInstantiatedDerivedType( + vectorDerivedType, DeclTypeSpec::Category::TypeDerived)}) { // This derived type and parameter expressions (if any) are already present // in the __ppc_intrinsics scope. SetDeclTypeSpec(*extant); @@ -5934,7 +5934,7 @@ bool DeclarationVisitor::Pre(const parser::DeclarationTypeSpec::Type &) { void DeclarationVisitor::Post(const parser::DeclarationTypeSpec::Type &type) { const parser::Name &derivedName{std::get<parser::Name>(type.derived.t)}; - if (const Symbol * derivedSymbol{derivedName.symbol}) { + if (const Symbol *derivedSymbol{derivedName.symbol}) { CheckForAbstractType(*derivedSymbol); // C706 } } @@ -6003,8 +6003,8 @@ void DeclarationVisitor::Post(const parser::DerivedTypeSpec &x) { if (!spec->MightBeParameterized()) { spec->EvaluateParameters(context()); } - if (const DeclTypeSpec * - extant{currScope().FindInstantiatedDerivedType(*spec, category)}) { + if (const DeclTypeSpec *extant{ + currScope().FindInstantiatedDerivedType(*spec, category)}) { // This derived type and parameter expressions (if any) are already present // in this scope. SetDeclTypeSpec(*extant); @@ -6035,8 +6035,7 @@ void DeclarationVisitor::Post(const parser::DeclarationTypeSpec::Record &rec) { if (auto spec{ResolveDerivedType(typeName)}) { spec->CookParameters(GetFoldingContext()); ... [truncated] |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR implements implicit generation of default OpenMP declare mappers for derived types (including allocatable ones) when no user-supplied mapper is provided, updating tests and lowering passes to exercise and emit these mappers.
- Added FileCheck tests for implicit declare mappers in
derived-type-map.f90 - Introduced
genImplicitDefaultDeclareMapperin the OpenMP lowering to emitDeclareMapperOpregions - Wired through the new mapper IDs in the map info finalization and clause processing
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| flang/test/Lower/OpenMP/derived-type-map.f90 | Added CHECK lines for implicit declare mappers |
| flang/lib/Semantics/resolve-names.cpp | Minor brace-formatting cleanup |
| flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp | Use the real mapperIdAttr instead of a blank attr |
| flang/lib/Lower/OpenMP/OpenMP.cpp | Added genImplicitDefaultDeclareMapper and hook up mapper IDs |
| flang/lib/Lower/OpenMP/ClauseProcessor.cpp | Adjusted default-mapper lookup and avoid self-mapping |
Comments suppressed due to low confidence (2)
flang/test/Lower/OpenMP/derived-type-map.f90:23
- The
uniq_nameusesEscalarinstead ofScalar. Correcting this typo will keep naming consistent with the derived typescalar_and_array.
!CHECK: %[[ALLOCA:.*]] = fir.alloca !fir.box<!fir.heap<!fir.type<_QFmaptype_derived_implicit_allocatableTscalar_and_array{real:f32,array:!fir.array<10xi32>,int:i32}>>> {bindc_name = "scalar_arr", uniq_name = "_QFmaptype_derived_implicit_allocatableEscalar_arr"} flang/lib/Lower/OpenMP/OpenMP.cpp:2351
- [nitpick] Consider adding a brief comment above
genImplicitDefaultDeclareMapperto explain its purpose, inputs, and outputs to improve readability and maintainability.
static mlir::FlatSymbolRefAttr 
| ✅ With the latest revision this PR passed the C/C++ code formatter. |
TIFitis force-pushed the users/Akash/implicit_default_mapper branch from 5d735f1 to 580e862 Compare 
skatrak reviewed Jul 1, 2025
Member
skatrak left a comment
skatrak left a comment There was a problem hiding this comment.
Thank you Akash for this work, and excuse the delay!
TIFitis force-pushed the users/Akash/implicit_default_mapper branch from 99a489d to 138250f Compare skatrak approved these changes Jul 25, 2025
Member
skatrak left a comment
skatrak left a comment There was a problem hiding this comment.
Thank you Akash for addressing my comments, and please excuse the delay taking a second look! LGTM.
agozillon approved these changes Jul 25, 2025
Contributor
agozillon left a comment
agozillon left a comment There was a problem hiding this comment.
Hey Akash, this LGTM.
However, could you please add a test to the map-types-and-sizes.f90 test or create an equivalent for the mapper work! Would be good to keep track of what we expect the lowered LLVM-IR to be.
I also don't believe this PR handles more complicated mapping cases like below (please do correct me if I am wrong):
program main type :: bottom_layer integer(4), allocatable :: array_j3(:) end type bottom_layer type :: middle_layer type(bottom_layer), allocatable :: nest real(4), allocatable :: array_j2(:) end type middle_layer type :: top_layer integer, allocatable :: array_j(:) type(middle_layer), allocatable :: nested end type top_layer type(top_layer), allocatable :: top_dtype allocate(top_dtype) allocate(top_dtype%nested) allocate(top_dtype%nested%nest) allocate(top_dtype%array_j(10)) allocate(top_dtype%nested%array_j2(10)) allocate(top_dtype%nested%nest%array_j3(10)) !$omp target top_dtype%array_j(1) = 10 top_dtype%nested%array_j2(1) = 20 top_dtype%nested%nest%array_j3(1) = 30 !$omp end target print *, top_dtype%nested%nest%array_j3(1) print *, top_dtype%nested%array_j2(1) print *, top_dtype%array_j(1) end program main How difficult would it be to extend this work to handle the above cases? As I think that'd be the ideal next step for this work!
A really nice end goal for this work would be to handle slightly more complicated case than even the above with arbitrary nestings of array of allocatable derived types with allocatable types (arrays, scalars etc.)!
flang/lib/Lower/OpenMP/OpenMP.cpp Outdated
| /*contiguousHint=*/true) | ||
| .first; | ||
| fir::factory::AddrAndBoundsInfo info = fir::factory::getDataOperandBaseAddr( | ||
| firOpBuilder, mapVal, /*isOptional=*/false, mapVal.getLoc()); |
Contributor
There was a problem hiding this comment.
I wonder if this would break if the input record type to a function was optional and we opted to not provide the argument, ran into a similar issue a while ago. Not the biggest deal for this PR, but good idea to test it at some point!
TIFitis force-pushed the users/Akash/implicit_default_mapper branch from c2ca1e8 to 9dded5e Compare llvmbot added libc++ TIFitis force-pushed the users/Akash/implicit_default_mapper branch from 1c9de45 to cdf4af1 Compare Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ypes This patch adds support to emit default declare mappers for implicit mapping of derived types when not supplied by user. This especially helps tackle mapping of allocatables of derived types. This supports nested derived types as well.
TIFitis force-pushed the users/Akash/implicit_default_mapper branch from cdf4af1 to 769c89a Compare 
Member Author
| @skatrak @agozillon Thanks for your reviews previously. I had to put this patch on hold as they example that Andrew had shared above didn't work and needed bits of other supporting work to go in before. Those works have now been upstreamed so we can revive this PR. The core of the code is mostly unchanged since it was last accepted. I have moved the getOrGenImplicitDefaultDeclareMapper to the flang/lib/Lower/OpenMP/Utils.cpp file from flang/lib/Lower/OpenMP/OpenMP.cpp. And I have had to added some supporting changes in ClauseProcessor to keep all the offloading tests stable while also providing the functionality we want from this PR. As is, Andrew's example above compiles and produces correct output. Would be great if you could green light this PR for merge if you're both happy with the changes. Thanks. |
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
agozillon reviewed Nov 13, 2025
Contributor
agozillon left a comment
agozillon left a comment There was a problem hiding this comment.
The PR LGTM so far, just a few questions and a NIT or two. And a request for one or two offload tests in check-offload if at all possible, just so we can see if there's any runtime regressions.
I was also wondering if this work would allow us to remove this segment of code: https://github.com/llvm/llvm-project/blob/main/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp#L948
I did test removing it and keeping just your PR in with the following example:
program main
implicit none
integer:: i
type :: bottom_layer real(4) :: i integer, allocatable :: scalar_i integer(4) :: array_i(10) integer, allocatable :: array_k(:) integer(4) :: k end type bottom_layer type :: one_layer real(4) :: i integer, allocatable :: scalar integer(4) :: array_i(10) real(4) :: j integer, allocatable :: array_j(:) integer(4) :: k type(bottom_layer) :: nest end type one_layer type(one_layer) :: one_l allocate(one_l%nest%array_k(10)) allocate(one_l%array_j(10)) allocate(one_l%nest%scalar_i) allocate(one_l%scalar) do i = 1, 10 one_l%nest%array_i(i) = i end do one_l%nest%scalar_i = 50 one_l%scalar = 25 !$omp target do i = 1, 10 one_l%nest%array_k(i) = one_l%nest%array_i(i) + i + one_l%nest%scalar_i + one_l%scalar one_l%array_j(i) = one_l%nest%array_k(i) end do !$omp end target print *, one_l%nest%array_k
print *, one_l%nest%array_i
print *, one_l%array_j
end program
Unfortunately, it seems that the MapInfoFinalization segment is still required for it, as there's unfortunately no declare mappers generated in this case (from what I could see when testing at least). Is this case something we could perhaps support with the declare mapper approach so we could unify the handling of implicit derived type member mapping? Not sure if it's a trivial extension or something that'd require a fair bit of legwork, if it's the latter we can likely leave it for now :-)
| (objectTypeSpec && | ||
| requiresImplicitDefaultDeclareMapper(*objectTypeSpec)); | ||
| bool containsDelete = (mapTypeBits & mlir::omp::ClauseMapFlags::del) != | ||
| mlir::omp::ClauseMapFlags::none; |
Contributor
There was a problem hiding this comment.
Perhaps a dumb question/thought, wouldn't we want to still emit in these cases? As we've implicitly allocated, I imagine implicit deallocation should perhaps still be done to prevent a user accidentally leaving bits and pieces on device, I could be wrong though!
| firOpBuilder, converter, defaultMaps, eleType, loc, sym); | ||
| | ||
| mlir::FlatSymbolRefAttr mapperId; | ||
| if (defaultMaps.empty()) { |
Contributor
There was a problem hiding this comment.
Might be good in the future to integrate this a little more elegantly with defaultmaps! As I imagine we don't want to prevent implicit mapping just because someone has specified a non-related implicit default map type (e.g. defaultmap(scalar: tofrom), and we might also want to take into account the user specified default mapping for either allocatable/pointer/aggregates, although not too sure where the specification stands on where each mapping would fall into or if they'd all fall under aggregate :-)
But not the biggest deal at the moment for this PR, perhaps a good idea to add a TODO for it here though.
| /*uniq_name=*/""); | ||
| | ||
| const auto genBoundsOps = [&](mlir::Value mapVal, | ||
| llvm::SmallVectorImpl<mlir::Value> &bounds) { |
Contributor
There was a problem hiding this comment.
I'm not 100% sure on this, but we may have a utility function for this lying around somewhere that might be able to replace this, could be worth looking into. If not it's likely something we really should add at some point, there's a lot of locations trying to do similar things.
| llvm::SmallVector<mlir::Value> memberMapOps; | ||
| | ||
| mlir::omp::ClauseMapFlags mapFlag = mlir::omp::ClauseMapFlags::to; | ||
| mapFlag |= mlir::omp::ClauseMapFlags::from; |
Contributor
There was a problem hiding this comment.
Nit: could probably just combine the mapFlag into one line via "|"
| | ||
| llvm::SmallVector<mlir::Value> bounds; | ||
| genBoundsOps(declareOp.getOriginalBase(), bounds); | ||
| mlir::omp::ClauseMapFlags parentMapFlag = mlir::omp::ClauseMapFlags::to; |
Contributor
There was a problem hiding this comment.
Nit: Same as above can combine into a single line
| // Once explicit members are attached to a parent map, do not also invoke | ||
| // a declare mapper on it, otherwise the mapper would remap the same | ||
| // components leading to duplicate mappings at runtime. | ||
| if (!indices.second.memberMap.empty() && mapOp.getMapperIdAttr()) |
Contributor
There was a problem hiding this comment.
Perhaps a dumb question, would this prevent mapping of scalar in the below example (excuse the pseudo code!):
type :: dtype2
integer, allocatable :: scalar
end type :: dtype2
type :: dtype real(4) :: i dtype2 :: dtype2_v end type dtype dtype :: dtype_v !$omp target map(dtype_v%i, dtype_2_v)
!$omp end target
As I think we'd still want to map it in these scenarios as far as I'm aware at least, Michael Klemm may know better.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
This patch adds support to emit default declare mappers for implicit mapping of derived types when not supplied by user. This especially helps tackle mapping of allocatables of derived types.