From 2af3ef49bedcf4051a35d16f5f8696fc4cf35dba Mon Sep 17 00:00:00 2001 From: agozillon Date: Mon, 3 Nov 2025 15:03:18 -0600 Subject: [PATCH 1/8] WIP ref_* map type --- flang/lib/Lower/OpenMP/ClauseProcessor.cpp | 14 + flang/lib/Lower/OpenMP/OpenMP.cpp | 38 +- .../Optimizer/OpenMP/MapInfoFinalization.cpp | 514 +++++++++++------- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | 4 +- .../OpenMP/OpenMPToLLVMIRTranslation.cpp | 54 +- mlir/lib/Target/LLVMIR/ModuleTranslation.cpp | 2 + 6 files changed, 410 insertions(+), 216 deletions(-) diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp index 9cdd46137adbf..0edcea9097026 100644 --- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp +++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp @@ -1357,6 +1357,20 @@ bool ClauseProcessor::processMap( mapTypeBits |= mlir::omp::ClauseMapFlags::ompx_hold; } + if (refMod) { + switch (*refMod) { + case Map::RefModifier::RefPtee: + mapTypeBits |= mlir::omp::ClauseMapFlags::ref_ptee; + break; + case Map::RefModifier::RefPtr: + mapTypeBits |= mlir::omp::ClauseMapFlags::ref_ptr; + break; + case Map::RefModifier::RefPtrPtee: + mapTypeBits |= mlir::omp::ClauseMapFlags::ref_ptr_ptee; + break; + } + } + if (iterator) { TODO(currentLocation, "Support for iterator modifiers is not implemented yet"); diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp index 1f7084ab4315d..3c743f52f0a8f 100644 --- a/flang/lib/Lower/OpenMP/OpenMP.cpp +++ b/flang/lib/Lower/OpenMP/OpenMP.cpp @@ -368,7 +368,6 @@ extractMappedBaseValues(llvm::ArrayRef vars, llvm::transform(vars, std::back_inserter(baseOps), [](mlir::Value map) { auto mapInfo = map.getDefiningOp(); assert(mapInfo && "expected all map vars to be defined by omp.map.info"); - mlir::Value varPtr = mapInfo.getVarPtr(); if (auto boxAddr = varPtr.getDefiningOp()) return boxAddr.getVal(); @@ -2522,6 +2521,39 @@ static bool isDuplicateMappedSymbol( return checkSymbol(sym.GetUltimate()); } +// TODO: If this works and we go this route, we need to cache the +// default mappers for each type and that we're binding to the +// block args... +// +// Doesn't quite work without sticking a map operation in and that seems like +// doing the same as Map None with extra steps... +// static mlir::FlatSymbolRefAttr +// getOrGenImplicitNoneDeclareMapper(lower::AbstractConverter &converter, +// mlir::Location loc, mlir::Type type, +// llvm::StringRef mapperNameStr) { +// if (converter.getModuleOp().lookupSymbol(mapperNameStr)) +// return mlir::FlatSymbolRefAttr::get(&converter.getMLIRContext(), +// mapperNameStr); + +// fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder(); + +// // 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 = mlir::omp::DeclareMapperOp::create(firOpBuilder, loc, +// mapperNameStr, type); +// auto ®ion = declMapperOp.getRegion(); +// firOpBuilder.createBlock(®ion); + +// mlir::omp::DeclareMapperInfoOp::create(firOpBuilder, loc, mlir::Value{}); + +// declMapperOp.dump(); +// return mlir::FlatSymbolRefAttr::get(&converter.getMLIRContext(), +// mapperNameStr); +// } + static mlir::omp::TargetOp genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable, lower::StatementContext &stmtCtx, @@ -2626,7 +2658,8 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable, mlir::Value mapOp = createMapInfoOp( firOpBuilder, converter.getCurrentLocation(), baseOp, /*varPtrPtr=*/mlir::Value{}, name.str(), bounds, /*members=*/{}, - /*membersIndex=*/mlir::ArrayAttr{}, std::get<0>(mapFlagAndKind), + /*membersIndex=*/mlir::ArrayAttr{}, + std::get<0>(mapFlagAndKind), std::get<1>(mapFlagAndKind), baseOp.getType(), /*partialMap=*/false, mapperId); @@ -2634,6 +2667,7 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable, mapSyms.push_back(&sym); } }; + lower::pft::visitAllSymbols(eval, captureImplicitMap); auto targetOp = mlir::omp::TargetOp::create(firOpBuilder, loc, clauseOps); diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp index ee4b56d5c7d5f..c863da11d7863 100644 --- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp +++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp @@ -443,6 +443,162 @@ class MapInfoFinalizationPass baseAddrIndex); } + // We add all mapped record members not directly used in the target region + // to the block arguments in front of their parent and we place them into + // the map operands list for consistency. + // + // These indirect uses (via accesses to their parent) will still be + // mapped individually in most cases, and a parent mapping doesn't + // guarantee the parent will be mapped in its totality, partial + // mapping is common. + // + // For example: + // map(tofrom: x%y) + // + // Will generate a mapping for "x" (the parent) and "y" (the member). + // The parent "x" will not be mapped, but the member "y" will. + // However, we must have the parent as a BlockArg and MapOperand + // in these cases, to maintain the correct uses within the region and + // to help tracking that the member is part of a larger object. + // + // In the case of: + // map(tofrom: x%y, x%z) + // + // The parent member becomes more critical, as we perform a partial + // structure mapping where we link the mapping of the members y + // and z together via the parent x. We do this at a kernel argument + // level in LLVM IR and not just MLIR, which is important to maintain + // similarity to Clang and for the runtime to do the correct thing. + // However, we still do not map the structure in its totality but + // rather we generate an un-sized "binding" map entry for it. + // + // In the case of: + // map(tofrom: x, x%y, x%z) + // + // We do actually map the entirety of "x", so the explicit mapping of + // x%y, x%z becomes unnecessary. It is redundant to write this from a + // Fortran OpenMP perspective (although it is legal), as even if the + // members were allocatables or pointers, we are mandated by the + // specification to map these (and any recursive components) in their + // entirety, which is different to the C++ equivalent, which requires + // explicit mapping of these segments. + void addImplicitMembersToTarget(mlir::omp::MapInfoOp op, + fir::FirOpBuilder &builder, + mlir::Operation *target, + bool insertMapOp = false) { + auto mapClauseOwner = + llvm::dyn_cast_if_present( + target); + + // TargetDataOp is technically a MapClauseOwningOpInterface, so we + // do not need to explicitly check for the extra cases here for use_device + // addr/ptr + if (!mapClauseOwner) + return; + + auto addOperands = [&](mlir::MutableOperandRange &mapVarsArr, + mlir::Operation *directiveOp, + unsigned blockArgInsertIndex = 0) { + if (!llvm::is_contained(mapVarsArr.getAsOperandRange(), op.getResult()) && + !insertMapOp) + return; + + llvm::SmallVector newMapOps; + auto insertMap = [&](mlir::Value newInsert) { + newMapOps.push_back(newInsert); + if (directiveOp) { + directiveOp->getRegion(0).insertArgument( + blockArgInsertIndex, newInsert.getType(), newInsert.getLoc()); + blockArgInsertIndex++; + } + }; + + // There doesn't appear to be a simple way to convert MutableOperandRange + // to a vector currently, so we instead use a for_each to populate our + // vector. + newMapOps.reserve(mapVarsArr.size()); + llvm::for_each( + mapVarsArr.getAsOperandRange(), + [&newMapOps](mlir::Value oper) { newMapOps.push_back(oper); }); + + // if the map doesn't exist on the target already, then we add it. + if (!llvm::is_contained(mapVarsArr.getAsOperandRange(), op.getResult())) + insertMap(op); + + // Insert all of the members that the map contains into the new target map + // list. + for (auto mapMember : op.getMembers()) { + if (llvm::is_contained(mapVarsArr.getAsOperandRange(), mapMember)) + continue; + insertMap(mapMember); + } + + mapVarsArr.assign(newMapOps); + }; + + auto argIface = + llvm::dyn_cast(target); + + if (auto mapClauseOwner = + llvm::dyn_cast(target)) { + mlir::MutableOperandRange mapVarsArr = mapClauseOwner.getMapVarsMutable(); + unsigned blockArgInsertIndex = + argIface + ? argIface.getMapBlockArgsStart() + argIface.numMapBlockArgs() + : 0; + addOperands(mapVarsArr, + llvm::dyn_cast_if_present( + argIface.getOperation()), + blockArgInsertIndex); + } + + if (auto targetDataOp = llvm::dyn_cast(target)) { + mlir::MutableOperandRange useDevAddrMutableOpRange = + targetDataOp.getUseDeviceAddrVarsMutable(); + addOperands(useDevAddrMutableOpRange, target, + argIface.getUseDeviceAddrBlockArgsStart() + + argIface.numUseDeviceAddrBlockArgs()); + + mlir::MutableOperandRange useDevPtrMutableOpRange = + targetDataOp.getUseDevicePtrVarsMutable(); + addOperands(useDevPtrMutableOpRange, target, + argIface.getUseDevicePtrBlockArgsStart() + + argIface.numUseDevicePtrBlockArgs()); + } else if (auto targetOp = llvm::dyn_cast(target)) { + mlir::MutableOperandRange hasDevAddrMutableOpRange = + targetOp.getHasDeviceAddrVarsMutable(); + addOperands(hasDevAddrMutableOpRange, target, + argIface.getHasDeviceAddrBlockArgsStart() + + argIface.numHasDeviceAddrBlockArgs()); + } + } + + // We retrieve the first user that is a Target operation, of which + // there should only be one currently. Every MapInfoOp can be tied to + // at most one Target operation and at the minimum no operations. + // This may change in the future with IR cleanups/modifications, + // in which case this pass will need updating to support cases + // where a map can have more than one user and more than one of + // those users can be a Target operation. For now, we simply + // return the first target operation encountered, which may + // be on the parent MapInfoOp in the case of a member mapping. + // In that case, we traverse the MapInfoOp chain until we + // find the first TargetOp user. + mlir::Operation *getFirstTargetUser(mlir::omp::MapInfoOp mapOp) { + for (auto *user : mapOp->getUsers()) { + if (llvm::isa(user)) + return user; + + if (auto mapUser = llvm::dyn_cast(user)) + return getFirstTargetUser(mapUser); + } + + return nullptr; + } + /// Adjusts the descriptor's map type. The main alteration that is done /// currently is transforming the map type to `OMP_MAP_TO` where possible. /// This is because we will always need to map the descriptor to device @@ -471,7 +627,13 @@ class MapInfoFinalizationPass mlir::Operation *target, bool isHasDeviceAddr) { using mapFlags = mlir::omp::ClauseMapFlags; mapFlags flags = mapFlags::none; - if (!isHasDeviceAddr) + + // Don't apply attach to map exiting directives where trivially + // possible. Makes the emitted IR simpler, but shouldn't affect + // correctness as the runtime ignores the extra attach maps we + // generate in these scenarios. + if (!isHasDeviceAddr && + !llvm::isa_and_nonnull(target)) flags |= mapFlags::attach; if (llvm::isa_and_nonnullgetLoc(), descMapOp.getResult().getType(), + descMapOp.getVarPtr(), descMapOp.getVarTypeAttr(), + builder.getAttr( + mlir::omp::ClauseMapFlags::attach | refFlagType), + descMapOp.getMapCaptureTypeAttr(), /*varPtrPtr=*/ + fir::BoxOffsetOp::create(builder, descMapOp->getLoc(), descriptor, + fir::BoxFieldAttr::base_addr), + descMapOp.getMembers(), descMapOp.getMembersIndexAttr(), + /*bounds=*/mlir::SmallVector{}, + /*mapperId*/ mlir::FlatSymbolRefAttr(), descMapOp.getNameAttr(), + /*partial_map=*/builder.getBoolAttr(false)); + + // Has to be added to the target immediately, as we expect all maps + // processed by this pass to have a user that is a target. + addImplicitMembersToTarget(implicitAttachMap, builder, target, true); + } + // Expand mappings of type(C_PTR) to map their `__address` field explicitly // as a single pointer-sized member (USM-gated at callsite). This helps in // USM scenarios to ensure the pointer-sized mapping is used. @@ -586,28 +769,6 @@ class MapInfoFinalizationPass return newParent; } - mlir::omp::MapInfoOp genDescriptorMemberMaps(mlir::omp::MapInfoOp op, - fir::FirOpBuilder &builder, - mlir::Operation *target) { - llvm::SmallVector mapMemberUsers; - getMemberUserList(op, mapMemberUsers); - - // TODO: map the addendum segment of the descriptor, similarly to the - // base address/data pointer member. - bool descCanBeDeferred = false; - mlir::Value descriptor = - getDescriptorFromBoxMap(op, builder, descCanBeDeferred); - - mlir::ArrayAttr newMembersAttr; - mlir::SmallVector newMembers; - llvm::SmallVector> memberIndices; - bool isHasDeviceAddrFlag = isHasDeviceAddr(op, *target); - - if (!mapMemberUsers.empty() || !op.getMembers().empty()) - getMemberIndicesAsVectors( - !mapMemberUsers.empty() ? mapMemberUsers[0].parent : op, - memberIndices); - // If the operation that we are expanding with a descriptor has a user // (parent), then we have to expand the parent's member indices to reflect // the adjusted member indices for the base address insertion. However, if @@ -621,6 +782,14 @@ class MapInfoFinalizationPass // from the descriptor to be used verbatim, i.e. without additional // remapping. To avoid this remapping, simply don't generate any map // information for the descriptor members. + void createBaseAddrInsertion( + fir::FirOpBuilder &builder, mlir::omp::MapInfoOp parentOp, + mlir::omp::MapInfoOp baseAddr, + llvm::SmallVectorImpl &mapMemberUsers, + mlir::ArrayAttr &newMembersAttr, + mlir::SmallVectorImpl &newMembers, + llvm::SmallVector> &memberIndices, + bool isHasDeviceAddrFlag) { if (!mapMemberUsers.empty()) { // Currently, there should only be one user per map when this pass // is executed. Either a parent map, holding the current map in its @@ -631,194 +800,165 @@ class MapInfoFinalizationPass assert(mapMemberUsers.size() == 1 && "OMPMapInfoFinalization currently only supports single users of a " "MapInfoOp"); - auto baseAddr = - genBaseAddrMap(descriptor, op.getBounds(), op.getMapType(), builder); ParentAndPlacement mapUser = mapMemberUsers[0]; adjustMemberIndices(memberIndices, mapUser); llvm::SmallVector newMemberOps; for (auto v : mapUser.parent.getMembers()) { newMemberOps.push_back(v); - if (v == op) + if (v == parentOp) newMemberOps.push_back(baseAddr); } mapUser.parent.getMembersMutable().assign(newMemberOps); mapUser.parent.setMembersIndexAttr( builder.create2DI64ArrayAttr(memberIndices)); } else if (!isHasDeviceAddrFlag) { - auto baseAddr = - genBaseAddrMap(descriptor, op.getBounds(), op.getMapType(), builder); newMembers.push_back(baseAddr); - if (!op.getMembers().empty()) { + if (!parentOp.getMembers().empty()) { for (auto &indices : memberIndices) indices.insert(indices.begin(), 0); memberIndices.insert(memberIndices.begin(), {0}); newMembersAttr = builder.create2DI64ArrayAttr(memberIndices); - newMembers.append(op.getMembers().begin(), op.getMembers().end()); + newMembers.append(parentOp.getMembers().begin(), + parentOp.getMembers().end()); } else { llvm::SmallVector> memberIdx = {{0}}; newMembersAttr = builder.create2DI64ArrayAttr(memberIdx); } } - - mlir::omp::MapInfoOp newDescParentMapOp = mlir::omp::MapInfoOp::create( - builder, op->getLoc(), op.getResult().getType(), descriptor, - mlir::TypeAttr::get(fir::unwrapRefType(descriptor.getType())), - builder.getAttr( - getDescriptorMapType(op.getMapType(), target, isHasDeviceAddrFlag)), - op.getMapCaptureTypeAttr(), /*varPtrPtr=*/mlir::Value{}, newMembers, - newMembersAttr, /*bounds=*/mlir::SmallVector{}, - /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), - /*partial_map=*/builder.getBoolAttr(false)); - op.replaceAllUsesWith(newDescParentMapOp.getResult()); - op->erase(); - - if (descCanBeDeferred) - deferrableDesc.push_back(newDescParentMapOp); - - return newDescParentMapOp; - } - - // We add all mapped record members not directly used in the target region - // to the block arguments in front of their parent and we place them into - // the map operands list for consistency. - // - // These indirect uses (via accesses to their parent) will still be - // mapped individually in most cases, and a parent mapping doesn't - // guarantee the parent will be mapped in its totality, partial - // mapping is common. - // - // For example: - // map(tofrom: x%y) - // - // Will generate a mapping for "x" (the parent) and "y" (the member). - // The parent "x" will not be mapped, but the member "y" will. - // However, we must have the parent as a BlockArg and MapOperand - // in these cases, to maintain the correct uses within the region and - // to help tracking that the member is part of a larger object. - // - // In the case of: - // map(tofrom: x%y, x%z) - // - // The parent member becomes more critical, as we perform a partial - // structure mapping where we link the mapping of the members y - // and z together via the parent x. We do this at a kernel argument - // level in LLVM IR and not just MLIR, which is important to maintain - // similarity to Clang and for the runtime to do the correct thing. - // However, we still do not map the structure in its totality but - // rather we generate an un-sized "binding" map entry for it. - // - // In the case of: - // map(tofrom: x, x%y, x%z) - // - // We do actually map the entirety of "x", so the explicit mapping of - // x%y, x%z becomes unnecessary. It is redundant to write this from a - // Fortran OpenMP perspective (although it is legal), as even if the - // members were allocatables or pointers, we are mandated by the - // specification to map these (and any recursive components) in their - // entirety, which is different to the C++ equivalent, which requires - // explicit mapping of these segments. - void addImplicitMembersToTarget(mlir::omp::MapInfoOp op, - fir::FirOpBuilder &builder, - mlir::Operation *target) { - auto mapClauseOwner = - llvm::dyn_cast_if_present( - target); - // TargetDataOp is technically a MapClauseOwningOpInterface, so we - // do not need to explicitly check for the extra cases here for use_device - // addr/ptr - if (!mapClauseOwner) - return; - - auto addOperands = [&](mlir::MutableOperandRange &mapVarsArr, - mlir::Operation *directiveOp, - unsigned blockArgInsertIndex = 0) { - if (!llvm::is_contained(mapVarsArr.getAsOperandRange(), op.getResult())) - return; - - // There doesn't appear to be a simple way to convert MutableOperandRange - // to a vector currently, so we instead use a for_each to populate our - // vector. - llvm::SmallVector newMapOps; - newMapOps.reserve(mapVarsArr.size()); - llvm::for_each( - mapVarsArr.getAsOperandRange(), - [&newMapOps](mlir::Value oper) { newMapOps.push_back(oper); }); - - for (auto mapMember : op.getMembers()) { - if (llvm::is_contained(mapVarsArr.getAsOperandRange(), mapMember)) - continue; - newMapOps.push_back(mapMember); - if (directiveOp) { - directiveOp->getRegion(0).insertArgument( - blockArgInsertIndex, mapMember.getType(), mapMember.getLoc()); - blockArgInsertIndex++; - } - } - - mapVarsArr.assign(newMapOps); - }; - - auto argIface = - llvm::dyn_cast(target); - - if (auto mapClauseOwner = - llvm::dyn_cast(target)) { - mlir::MutableOperandRange mapVarsArr = mapClauseOwner.getMapVarsMutable(); - unsigned blockArgInsertIndex = - argIface - ? argIface.getMapBlockArgsStart() + argIface.numMapBlockArgs() - : 0; - addOperands(mapVarsArr, - llvm::dyn_cast_if_present( - argIface.getOperation()), - blockArgInsertIndex); - } - - if (auto targetDataOp = llvm::dyn_cast(target)) { - mlir::MutableOperandRange useDevAddrMutableOpRange = - targetDataOp.getUseDeviceAddrVarsMutable(); - addOperands(useDevAddrMutableOpRange, target, - argIface.getUseDeviceAddrBlockArgsStart() + - argIface.numUseDeviceAddrBlockArgs()); - - mlir::MutableOperandRange useDevPtrMutableOpRange = - targetDataOp.getUseDevicePtrVarsMutable(); - addOperands(useDevPtrMutableOpRange, target, - argIface.getUseDevicePtrBlockArgsStart() + - argIface.numUseDevicePtrBlockArgs()); - } else if (auto targetOp = llvm::dyn_cast(target)) { - mlir::MutableOperandRange hasDevAddrMutableOpRange = - targetOp.getHasDeviceAddrVarsMutable(); - addOperands(hasDevAddrMutableOpRange, target, - argIface.getHasDeviceAddrBlockArgsStart() + - argIface.numHasDeviceAddrBlockArgs()); - } } - // We retrieve the first user that is a Target operation, of which - // there should only be one currently. Every MapInfoOp can be tied to - // at most one Target operation and at the minimum no operations. - // This may change in the future with IR cleanups/modifications, - // in which case this pass will need updating to support cases - // where a map can have more than one user and more than one of - // those users can be a Target operation. For now, we simply - // return the first target operation encountered, which may - // be on the parent MapInfoOp in the case of a member mapping. - // In that case, we traverse the MapInfoOp chain until we - // find the first TargetOp user. - mlir::Operation *getFirstTargetUser(mlir::omp::MapInfoOp mapOp) { - for (auto *user : mapOp->getUsers()) { - if (llvm::isa(user)) - return user; + void genDescriptorMaps(mlir::omp::MapInfoOp op, fir::FirOpBuilder &builder, + mlir::Operation *target) { + bool descCanBeDeferred = false; + llvm::SmallVector mapMemberUsers; + getMemberUserList(op, mapMemberUsers); - if (auto mapUser = llvm::dyn_cast(user)) - return getFirstTargetUser(mapUser); + // TODO: map the addendum segment of the descriptor, similarly to the + // base address/data pointer member. + bool isHasDeviceAddrFlag = isHasDeviceAddr(op, *target); + mlir::Value descriptor = + getDescriptorFromBoxMap(op, builder, descCanBeDeferred); + if ((op.getMapType() & mlir::omp::ClauseMapFlags::ref_ptr) == + mlir::omp::ClauseMapFlags::ref_ptr) { + // For ref_ptr, we generate a map of the descriptor with user specified map types and + // in the default auto attach case, we generate an additional attach map which indicates + // to the runtime to try and attach the base address to the descriptor if it's available + // and it's the first time the ref_ptr has been allocated on the device. + auto newMapInfoOp = mlir::omp::MapInfoOp::create( + builder, op->getLoc(), op.getResult().getType(), op.getVarPtr(), + op.getVarTypeAttr(), + builder.getAttr( + op.getMapType() & ~mlir::omp::ClauseMapFlags::ref_ptr), + op.getMapCaptureTypeAttr(), /*varPtrPtr=*/op.getVarPtrPtr(), + op.getMembers(), op.getMembersIndexAttr(), + /*bounds=*/mlir::SmallVector{}, + /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), + /*partial_map=*/builder.getBoolAttr(false)); + + // If we're a map exiting construct we skip the generation of the + // attach map, it should be unnecessary in these cases as it exists to + // bind the pointer and pointee and shouldn't increment or decrement + // the ref counter on its own. However, equally having it doesn't + // cause issues either, it's just ideal to remove the noise where + // feasible. + // TODO: Extend this to perhaps check for target updates and target data + // with release and from applied. + if (!llvm::isa(target)) + genImplicitAttachMap(op, descriptor, target, builder, + mlir::omp::ClauseMapFlags::ref_ptr); + op.replaceAllUsesWith(newMapInfoOp.getResult()); + op->erase(); + } else if ((op.getMapType() & mlir::omp::ClauseMapFlags::ref_ptee) == + mlir::omp::ClauseMapFlags::ref_ptee) { + + // TODO/FIX: + // 0) i) create two variations of the test with ref_ptr/ptee shuffled around + // ii) test ref_ptr_ptee + // iii) test some other ref_ptr/ptee examples (for example the stack descriptor getting stuck in memory example), and perhaps ask michael if he has any ideas + // - ask Michael what he thinks about turning off the descriptor deferral stuff for newer OpenMP versions by default + // and having to provide a flag in those cases. Just so we can nudge people towards using ref_ptr/ptee in those + // cases (and so we can test the ref_ptr/ptee test trivially) - Might not really matter for downstream as we have + // a flag to toggle it, but I don't think upstream has it + // iiiii) Make sure it works for all existing map tests and check-offload, check-smoke without breakages + // iiiiii) Add these tests to check-smoke with a script that will verify the runtime trace and check attaches are happening at the right time + // as well as transfers of the various bits and pieces... + // 1) Look into the possibility of teaching the backend to only require ATTACH map to do the more + // unique mapping of accessing varPtr/varPtrPtr fields for basePointer/offloadPointer as opposed + // to using the ref_ptee/ref_ptr types... + // 2) Remove the always map on the attach in lowering and try to find/fix the problem case so it's no + // longer required... + // 3) Implement attach always/auto/none + // - we'll want to apply ALWAYS when map always is given, and never apply when (perhaps in subsequent patch so as not to overload everything at once) attach none is provided, but the default case we will just generate this map. + // 4) Test and fix if neccessary the derived type case for ref_ptr/ptee where we apply it to multiple nested + // allocatables, the usual stuff needed to check and fix for descriptor derived type related stuff + // 5) look at tidying everything up more. + // 6) - Can we simplify the attach map AND the way we bind the descriptors by using ref_ptr_ptee to indicate + // we're a binding map, or would this cause issues? Or perhaps there's a better way to simplify the backend + // mappings by splitting them into 3 seperate maps in the else clause below, but it might cause issues for derived types + // need to test first.... + + // For ref_ptee, we generate a map of the base address with user specified map types and + // in the default auto attach case, we generate an additional attach map which indicates + // to the runtime to try and attach the base address to the descriptor if it's available + // and it's the first time the ref_ptee has been allocated on the device. + auto newMapInfoOp = genBaseAddrMap( + descriptor, op.getBounds(), + op.getMapType() & ~mlir::omp::ClauseMapFlags::ref_ptee, builder); + + // If we're a map exiting construct we skip the generation of the attach + // map, it should be unnecessary in these cases as it exists to bind the + // pointer and pointee and shouldn't increment or decrement the ref counter + // on its own. However, equally having it doesn't cause issues either, it's + // just ideal to remove the noise where feasible. + // TODO: Extend this to perhaps check for target updates and target data + // with release and from applied. + if (!llvm::isa(target)) + genImplicitAttachMap(op, descriptor, target, builder, + mlir::omp::ClauseMapFlags::ref_ptee); + op.replaceAllUsesWith(newMapInfoOp.getResult()); + op->erase(); + } else { + bool isRefPtrPtee = + (op.getMapType() & mlir::omp::ClauseMapFlags::ref_ptr_ptee) == + mlir::omp::ClauseMapFlags::ref_ptr_ptee; + + mlir::ArrayAttr newMembersAttr; + mlir::SmallVector newMembers; + llvm::SmallVector> memberIndices; + + if (!mapMemberUsers.empty() || !op.getMembers().empty()) + getMemberIndicesAsVectors( + !mapMemberUsers.empty() ? mapMemberUsers[0].parent : op, + memberIndices); + + mlir::omp::MapInfoOp baseAddr = + genBaseAddrMap(descriptor, op.getBounds(), op.getMapType(), builder); + createBaseAddrInsertion(builder, op, baseAddr, mapMemberUsers, + newMembersAttr, newMembers, memberIndices, + isHasDeviceAddrFlag); + + // If we have been provided RefPtrPtee, utilise the user specified map + // types as best we can only providing the additional map types necessary, + // otherwise, use the default descriptor map type. + auto mapType = + isRefPtrPtee ? (op.getMapType() | mlir::omp::ClauseMapFlags::attach | + mlir::omp::ClauseMapFlags::descriptor) + : getDescriptorMapType(op.getMapType(), target, + isHasDeviceAddrFlag); + auto newMapInfoOp = mlir::omp::MapInfoOp::create( + builder, op->getLoc(), op.getResult().getType(), descriptor, + mlir::TypeAttr::get(fir::unwrapRefType(descriptor.getType())), + builder.getAttr(mapType), + op.getMapCaptureTypeAttr(), /*varPtrPtr=*/mlir::Value{}, newMembers, + newMembersAttr, /*bounds=*/mlir::SmallVector{}, + /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), + /*partial_map=*/builder.getBoolAttr(false)); + op.replaceAllUsesWith(newMapInfoOp.getResult()); + op->erase(); + + if (descCanBeDeferred) + deferrableDesc.push_back(newMapInfoOp); } - - return nullptr; } void addImplicitDescriptorMapToTargetDataOp(mlir::omp::MapInfoOp op, @@ -1197,7 +1337,7 @@ class MapInfoFinalizationPass builder.setInsertionPoint(op); mlir::Operation *targetUser = getFirstTargetUser(op); assert(targetUser && "expected user of map operation was not found"); - genDescriptorMemberMaps(op, builder, targetUser); + genDescriptorMaps(op, builder, targetUser); } }); diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp index 172f21ff1779e..ad24ec7b48c62 100644 --- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp +++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp @@ -2049,8 +2049,8 @@ static LogicalResult verifyMapClause(Operation *op, OperandRange mapVars) { to ? updateToVars.insert(updateVar) : updateFromVars.insert(updateVar); } } else if (!isa(op)) { - return emitError(op->getLoc(), - "map argument is not a map entry operation"); + // return emitError(op->getLoc(), + // "map argument is not a map entry operation"); } } diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp index 242532e5879e6..2a1e4882d8d59 100644 --- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp +++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp @@ -3873,56 +3873,57 @@ static llvm::Value *getSizeInBytes(DataLayout &dl, const mlir::Type &type, return builder.getInt64(dl.getTypeSizeInBits(type) / 8); } +static bool checkHasClauseMapFlag(omp::ClauseMapFlags flag, + omp::ClauseMapFlags checkFlag) { + return (flag & checkFlag) == checkFlag; +} + // Convert the MLIR map flag set to the runtime map flag set for embedding // in LLVM-IR. This is important as the two bit-flag lists do not correspond // 1-to-1 as there's flags the runtime doesn't care about and vice versa. // Certain flags are discarded here such as RefPtee and co. static llvm::omp::OpenMPOffloadMappingFlags convertClauseMapFlags(omp::ClauseMapFlags mlirFlags) { - auto mapTypeToBool = [&mlirFlags](omp::ClauseMapFlags flag) { - return (mlirFlags & flag) == flag; - }; - llvm::omp::OpenMPOffloadMappingFlags mapType = llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE; - if (mapTypeToBool(omp::ClauseMapFlags::to)) + if (checkHasClauseMapFlag(mlirFlags, omp::ClauseMapFlags::to)) mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO; - if (mapTypeToBool(omp::ClauseMapFlags::from)) + if (checkHasClauseMapFlag(mlirFlags, omp::ClauseMapFlags::from)) mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM; - if (mapTypeToBool(omp::ClauseMapFlags::always)) + if (checkHasClauseMapFlag(mlirFlags, omp::ClauseMapFlags::always)) mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS; - if (mapTypeToBool(omp::ClauseMapFlags::del)) + if (checkHasClauseMapFlag(mlirFlags, omp::ClauseMapFlags::del)) mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_DELETE; - if (mapTypeToBool(omp::ClauseMapFlags::return_param)) + if (checkHasClauseMapFlag(mlirFlags, omp::ClauseMapFlags::return_param)) mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_RETURN_PARAM; - if (mapTypeToBool(omp::ClauseMapFlags::priv)) + if (checkHasClauseMapFlag(mlirFlags, omp::ClauseMapFlags::priv)) mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_PRIVATE; - if (mapTypeToBool(omp::ClauseMapFlags::literal)) + if (checkHasClauseMapFlag(mlirFlags, omp::ClauseMapFlags::literal)) mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_LITERAL; - if (mapTypeToBool(omp::ClauseMapFlags::implicit)) + if (checkHasClauseMapFlag(mlirFlags, omp::ClauseMapFlags::implicit)) mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_IMPLICIT; - if (mapTypeToBool(omp::ClauseMapFlags::close)) + if (checkHasClauseMapFlag(mlirFlags, omp::ClauseMapFlags::close)) mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_CLOSE; - if (mapTypeToBool(omp::ClauseMapFlags::present)) + if (checkHasClauseMapFlag(mlirFlags, omp::ClauseMapFlags::present)) mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_PRESENT; - if (mapTypeToBool(omp::ClauseMapFlags::ompx_hold)) + if (checkHasClauseMapFlag(mlirFlags, omp::ClauseMapFlags::ompx_hold)) mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_OMPX_HOLD; - if (mapTypeToBool(omp::ClauseMapFlags::attach)) + if (checkHasClauseMapFlag(mlirFlags, omp::ClauseMapFlags::attach)) mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ATTACH; - if (mapTypeToBool(omp::ClauseMapFlags::descriptor)) + if (checkHasClauseMapFlag(mlirFlags, omp::ClauseMapFlags::descriptor)) mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_DESCRIPTOR; return mapType; @@ -3954,10 +3955,16 @@ static void collectMapDataFromMapOperands( // Process MapOperands for (Value mapValue : mapVars) { auto mapOp = cast(mapValue.getDefiningOp()); - Value offloadPtr = - mapOp.getVarPtrPtr() ? mapOp.getVarPtrPtr() : mapOp.getVarPtr(); + bool isRefPtrOrPteeMapWithAttach = + checkHasClauseMapFlag(mapOp.getMapType(), + omp::ClauseMapFlags::ref_ptr) && + checkHasClauseMapFlag(mapOp.getMapType(), omp::ClauseMapFlags::attach); + Value offloadPtr = (mapOp.getVarPtrPtr() && !isRefPtrOrPteeMapWithAttach) ? mapOp.getVarPtrPtr() + : mapOp.getVarPtr(); mapData.OriginalValue.push_back(moduleTranslation.lookupValue(offloadPtr)); - mapData.Pointers.push_back(mapData.OriginalValue.back()); + mapData.Pointers.push_back( + isRefPtrOrPteeMapWithAttach ? moduleTranslation.lookupValue(mapOp.getVarPtrPtr()) + : mapData.OriginalValue.back()); // if is declare target link OR to/enter in USM mode if (llvm::Value *refPtr = @@ -4569,8 +4576,8 @@ void processAttachMap(LLVM::ModuleTranslation &moduleTranslation, // update it we can run into some unusual issues where the attachment is // still blocked. combinedInfo.Types.emplace_back( - llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ATTACH | - llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS); + llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ATTACH/* | + llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS*/); combinedInfo.DevicePointers.emplace_back( llvm::OpenMPIRBuilder::DeviceInfoTy::None); combinedInfo.Mappers.emplace_back(mapData.Mappers[mapDataIndex]); @@ -4960,8 +4967,6 @@ static void genMapInfos(llvm::IRBuilderBase &builder, // utilise the size from any component of MapInfoData, if we can't // something is missing from the initial MapInfoData construction. for (size_t i = 0; i < mapData.MapClause.size(); ++i) { - // NOTE/TODO: We currently do not support arbitrary depth record - // type mapping. if (mapData.IsAMember[i]) continue; @@ -4971,7 +4976,6 @@ static void genMapInfos(llvm::IRBuilderBase &builder, combinedInfo, mapData, i, targetDirective); continue; } - processIndividualMap(builder, *ompBuilder, mapData, i, combinedInfo, targetDirective); } diff --git a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp index 64e3c5f085bb3..4a36f6f39ab90 100644 --- a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp +++ b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp @@ -2394,6 +2394,8 @@ mlir::translateModuleToLLVMIR(Operation *module, llvm::LLVMContext &llvmContext, if (failed(translator.createDependentLibrariesMetadata())) return nullptr; + // module->dump(); + // Convert other top-level operations if possible. for (Operation &o : getModuleBody(module).getOperations()) { if (!isa Date: Fri, 7 Nov 2025 22:31:01 -0600 Subject: [PATCH 2/8] Tidy things up a bit more --- flang/lib/Lower/OpenMP/OpenMP.cpp | 38 +------------------ .../Optimizer/OpenMP/MapInfoFinalization.cpp | 2 +- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | 4 +- .../OpenMP/OpenMPToLLVMIRTranslation.cpp | 1 + mlir/lib/Target/LLVMIR/ModuleTranslation.cpp | 2 - 5 files changed, 6 insertions(+), 41 deletions(-) diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp index 3c743f52f0a8f..1f7084ab4315d 100644 --- a/flang/lib/Lower/OpenMP/OpenMP.cpp +++ b/flang/lib/Lower/OpenMP/OpenMP.cpp @@ -368,6 +368,7 @@ extractMappedBaseValues(llvm::ArrayRef vars, llvm::transform(vars, std::back_inserter(baseOps), [](mlir::Value map) { auto mapInfo = map.getDefiningOp(); assert(mapInfo && "expected all map vars to be defined by omp.map.info"); + mlir::Value varPtr = mapInfo.getVarPtr(); if (auto boxAddr = varPtr.getDefiningOp()) return boxAddr.getVal(); @@ -2521,39 +2522,6 @@ static bool isDuplicateMappedSymbol( return checkSymbol(sym.GetUltimate()); } -// TODO: If this works and we go this route, we need to cache the -// default mappers for each type and that we're binding to the -// block args... -// -// Doesn't quite work without sticking a map operation in and that seems like -// doing the same as Map None with extra steps... -// static mlir::FlatSymbolRefAttr -// getOrGenImplicitNoneDeclareMapper(lower::AbstractConverter &converter, -// mlir::Location loc, mlir::Type type, -// llvm::StringRef mapperNameStr) { -// if (converter.getModuleOp().lookupSymbol(mapperNameStr)) -// return mlir::FlatSymbolRefAttr::get(&converter.getMLIRContext(), -// mapperNameStr); - -// fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder(); - -// // 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 = mlir::omp::DeclareMapperOp::create(firOpBuilder, loc, -// mapperNameStr, type); -// auto ®ion = declMapperOp.getRegion(); -// firOpBuilder.createBlock(®ion); - -// mlir::omp::DeclareMapperInfoOp::create(firOpBuilder, loc, mlir::Value{}); - -// declMapperOp.dump(); -// return mlir::FlatSymbolRefAttr::get(&converter.getMLIRContext(), -// mapperNameStr); -// } - static mlir::omp::TargetOp genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable, lower::StatementContext &stmtCtx, @@ -2658,8 +2626,7 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable, mlir::Value mapOp = createMapInfoOp( firOpBuilder, converter.getCurrentLocation(), baseOp, /*varPtrPtr=*/mlir::Value{}, name.str(), bounds, /*members=*/{}, - /*membersIndex=*/mlir::ArrayAttr{}, - std::get<0>(mapFlagAndKind), + /*membersIndex=*/mlir::ArrayAttr{}, std::get<0>(mapFlagAndKind), std::get<1>(mapFlagAndKind), baseOp.getType(), /*partialMap=*/false, mapperId); @@ -2667,7 +2634,6 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable, mapSyms.push_back(&sym); } }; - lower::pft::visitAllSymbols(eval, captureImplicitMap); auto targetOp = mlir::omp::TargetOp::create(firOpBuilder, loc, clauseOps); diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp index c863da11d7863..ccadad52982bf 100644 --- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp +++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp @@ -443,7 +443,7 @@ class MapInfoFinalizationPass baseAddrIndex); } - // We add all mapped record members not directly used in the target region + // We add all mapped record members not directly used in the target region // to the block arguments in front of their parent and we place them into // the map operands list for consistency. // diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp index ad24ec7b48c62..172f21ff1779e 100644 --- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp +++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp @@ -2049,8 +2049,8 @@ static LogicalResult verifyMapClause(Operation *op, OperandRange mapVars) { to ? updateToVars.insert(updateVar) : updateFromVars.insert(updateVar); } } else if (!isa(op)) { - // return emitError(op->getLoc(), - // "map argument is not a map entry operation"); + return emitError(op->getLoc(), + "map argument is not a map entry operation"); } } diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp index 2a1e4882d8d59..c310b1c7dc1dd 100644 --- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp +++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp @@ -4976,6 +4976,7 @@ static void genMapInfos(llvm::IRBuilderBase &builder, combinedInfo, mapData, i, targetDirective); continue; } + processIndividualMap(builder, *ompBuilder, mapData, i, combinedInfo, targetDirective); } diff --git a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp index 4a36f6f39ab90..64e3c5f085bb3 100644 --- a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp +++ b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp @@ -2394,8 +2394,6 @@ mlir::translateModuleToLLVMIR(Operation *module, llvm::LLVMContext &llvmContext, if (failed(translator.createDependentLibrariesMetadata())) return nullptr; - // module->dump(); - // Convert other top-level operations if possible. for (Operation &o : getModuleBody(module).getOperations()) { if (!isa Date: Fri, 7 Nov 2025 23:03:20 -0600 Subject: [PATCH 3/8] Fix bug, apparently attach on exiting map directives are still required. --- .../Optimizer/OpenMP/MapInfoFinalization.cpp | 52 +++++-------------- 1 file changed, 14 insertions(+), 38 deletions(-) diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp index ccadad52982bf..a101184f82842 100644 --- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp +++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp @@ -628,12 +628,7 @@ class MapInfoFinalizationPass using mapFlags = mlir::omp::ClauseMapFlags; mapFlags flags = mapFlags::none; - // Don't apply attach to map exiting directives where trivially - // possible. Makes the emitted IR simpler, but shouldn't affect - // correctness as the runtime ignores the extra attach maps we - // generate in these scenarios. - if (!isHasDeviceAddr && - !llvm::isa_and_nonnull(target)) + if (!isHasDeviceAddr) flags |= mapFlags::attach; if (llvm::isa_and_nonnullgetLoc(), op.getResult().getType(), op.getVarPtr(), - op.getVarTypeAttr(), - builder.getAttr( - op.getMapType() & ~mlir::omp::ClauseMapFlags::ref_ptr), - op.getMapCaptureTypeAttr(), /*varPtrPtr=*/op.getVarPtrPtr(), - op.getMembers(), op.getMembersIndexAttr(), - /*bounds=*/mlir::SmallVector{}, - /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), - /*partial_map=*/builder.getBoolAttr(false)); - - // If we're a map exiting construct we skip the generation of the - // attach map, it should be unnecessary in these cases as it exists to - // bind the pointer and pointee and shouldn't increment or decrement - // the ref counter on its own. However, equally having it doesn't - // cause issues either, it's just ideal to remove the noise where - // feasible. - // TODO: Extend this to perhaps check for target updates and target data - // with release and from applied. - if (!llvm::isa(target)) - genImplicitAttachMap(op, descriptor, target, builder, - mlir::omp::ClauseMapFlags::ref_ptr); - op.replaceAllUsesWith(newMapInfoOp.getResult()); - op->erase(); + builder, op->getLoc(), op.getResult().getType(), op.getVarPtr(), + op.getVarTypeAttr(), + builder.getAttr( + op.getMapType() & ~mlir::omp::ClauseMapFlags::ref_ptr), + op.getMapCaptureTypeAttr(), /*varPtrPtr=*/op.getVarPtrPtr(), + op.getMembers(), op.getMembersIndexAttr(), + /*bounds=*/mlir::SmallVector{}, + /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), + /*partial_map=*/builder.getBoolAttr(false)); + genImplicitAttachMap(op, descriptor, target, builder, + mlir::omp::ClauseMapFlags::ref_ptr); + op.replaceAllUsesWith(newMapInfoOp.getResult()); + op->erase(); } else if ((op.getMapType() & mlir::omp::ClauseMapFlags::ref_ptee) == mlir::omp::ClauseMapFlags::ref_ptee) { @@ -904,15 +889,6 @@ class MapInfoFinalizationPass auto newMapInfoOp = genBaseAddrMap( descriptor, op.getBounds(), op.getMapType() & ~mlir::omp::ClauseMapFlags::ref_ptee, builder); - - // If we're a map exiting construct we skip the generation of the attach - // map, it should be unnecessary in these cases as it exists to bind the - // pointer and pointee and shouldn't increment or decrement the ref counter - // on its own. However, equally having it doesn't cause issues either, it's - // just ideal to remove the noise where feasible. - // TODO: Extend this to perhaps check for target updates and target data - // with release and from applied. - if (!llvm::isa(target)) genImplicitAttachMap(op, descriptor, target, builder, mlir::omp::ClauseMapFlags::ref_ptee); op.replaceAllUsesWith(newMapInfoOp.getResult()); From 938bf1ff99268eb45bc8cecf9e192cc3ad79fb9c Mon Sep 17 00:00:00 2001 From: agozillon Date: Fri, 7 Nov 2025 23:08:12 -0600 Subject: [PATCH 4/8] Re-add some of the exit directive attach map stuff for the moment as only one case was the issue --- .../Optimizer/OpenMP/MapInfoFinalization.cpp | 49 ++++++++++++++----- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp index a101184f82842..acf64c40c9e20 100644 --- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp +++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp @@ -840,19 +840,29 @@ class MapInfoFinalizationPass // to the runtime to try and attach the base address to the descriptor if it's available // and it's the first time the ref_ptr has been allocated on the device. auto newMapInfoOp = mlir::omp::MapInfoOp::create( - builder, op->getLoc(), op.getResult().getType(), op.getVarPtr(), - op.getVarTypeAttr(), - builder.getAttr( - op.getMapType() & ~mlir::omp::ClauseMapFlags::ref_ptr), - op.getMapCaptureTypeAttr(), /*varPtrPtr=*/op.getVarPtrPtr(), - op.getMembers(), op.getMembersIndexAttr(), - /*bounds=*/mlir::SmallVector{}, - /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), - /*partial_map=*/builder.getBoolAttr(false)); - genImplicitAttachMap(op, descriptor, target, builder, - mlir::omp::ClauseMapFlags::ref_ptr); - op.replaceAllUsesWith(newMapInfoOp.getResult()); - op->erase(); + builder, op->getLoc(), op.getResult().getType(), op.getVarPtr(), + op.getVarTypeAttr(), + builder.getAttr( + op.getMapType() & ~mlir::omp::ClauseMapFlags::ref_ptr), + op.getMapCaptureTypeAttr(), /*varPtrPtr=*/op.getVarPtrPtr(), + op.getMembers(), op.getMembersIndexAttr(), + /*bounds=*/mlir::SmallVector{}, + /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), + /*partial_map=*/builder.getBoolAttr(false)); + + // If we're a map exiting construct we skip the generation of the + // attach map, it should be unnecessary in these cases as it exists to + // bind the pointer and pointee and shouldn't increment or decrement + // the ref counter on its own. However, equally having it doesn't + // cause issues either, it's just ideal to remove the noise where + // feasible. + // TODO: Extend this to perhaps check for target updates and target data + // with release and from applied. + if (!llvm::isa(target)) + genImplicitAttachMap(op, descriptor, target, builder, + mlir::omp::ClauseMapFlags::ref_ptr); + op.replaceAllUsesWith(newMapInfoOp.getResult()); + op->erase(); } else if ((op.getMapType() & mlir::omp::ClauseMapFlags::ref_ptee) == mlir::omp::ClauseMapFlags::ref_ptee) { @@ -881,6 +891,10 @@ class MapInfoFinalizationPass // we're a binding map, or would this cause issues? Or perhaps there's a better way to simplify the backend // mappings by splitting them into 3 seperate maps in the else clause below, but it might cause issues for derived types // need to test first.... + // 7) - If 6 works then re-viist if we need attach maps on exit directives for the main combined map, it may have been tied to the fact the lowering + // found itn eccessary + // 8) Can perhaps revisit if we can detach the need for adding base_addresses to the members_of, but might be better to leave that + // to the future for a while as it's more of a tidying up scenario than a neccessity. // For ref_ptee, we generate a map of the base address with user specified map types and // in the default auto attach case, we generate an additional attach map which indicates @@ -889,6 +903,15 @@ class MapInfoFinalizationPass auto newMapInfoOp = genBaseAddrMap( descriptor, op.getBounds(), op.getMapType() & ~mlir::omp::ClauseMapFlags::ref_ptee, builder); + + // If we're a map exiting construct we skip the generation of the attach + // map, it should be unnecessary in these cases as it exists to bind the + // pointer and pointee and shouldn't increment or decrement the ref counter + // on its own. However, equally having it doesn't cause issues either, it's + // just ideal to remove the noise where feasible. + // TODO: Extend this to perhaps check for target updates and target data + // with release and from applied. + if (!llvm::isa(target)) genImplicitAttachMap(op, descriptor, target, builder, mlir::omp::ClauseMapFlags::ref_ptee); op.replaceAllUsesWith(newMapInfoOp.getResult()); From 64092a7d1f45eeae65f733bf017b0b70b42ce264 Mon Sep 17 00:00:00 2001 From: agozillon Date: Wed, 12 Nov 2025 17:41:51 -0600 Subject: [PATCH 5/8] [Flang] Fix defaultmap(none) being overly aggressive with symbol checks Currently we're picking up and complaining about builtin symbols like Null() when defaultmap(none) is set, so I've relaxed the restriction a bit to allow for procedures and named constants to bypass the restriction. It might be the case that we want to tighten it up again in certain aspects in the future. --- flang/lib/Semantics/resolve-directives.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp index 3744e43e98a28..939e8960d454c 100644 --- a/flang/lib/Semantics/resolve-directives.cpp +++ b/flang/lib/Semantics/resolve-directives.cpp @@ -3072,7 +3072,8 @@ void OmpAttributeVisitor::Post(const parser::Name &name) { if (Symbol * found{currScope().FindSymbol(name.source)}) { // If the variable has declare target applied to it (enter or link) it // is exempt from defaultmap(none) restrictions - if (!symbol->GetUltimate().test(Symbol::Flag::OmpDeclareTarget)) { + if (!symbol->GetUltimate().test(Symbol::Flag::OmpDeclareTarget) && + !IsProcedure(*symbol) && !IsNamedConstant(*symbol)) { auto &dMap = GetContext().defaultMap; for (auto defaults : dMap) { if (defaults.second == From f5d164458cf5724edcc4c4f3f52a3cca5bf5c68a Mon Sep 17 00:00:00 2001 From: agozillon Date: Wed, 12 Nov 2025 23:15:52 -0600 Subject: [PATCH 6/8] [Flang][OpenMP] WIP attach auto/never/always implementation --- flang/lib/Lower/OpenMP/ClauseProcessor.cpp | 16 ++- .../Optimizer/OpenMP/MapInfoFinalization.cpp | 128 ++++++++++++------ .../mlir/Dialect/OpenMP/OpenMPEnums.td | 4 +- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | 8 +- .../OpenMP/OpenMPToLLVMIRTranslation.cpp | 32 +++-- 5 files changed, 125 insertions(+), 63 deletions(-) diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp index 0edcea9097026..b1af11110b423 100644 --- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp +++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp @@ -1311,8 +1311,6 @@ bool ClauseProcessor::processMap( mlir::Location clauseLocation = converter.genLocation(source); const auto &[mapType, typeMods, attachMod, refMod, mappers, iterator, objects] = clause.t; - if (attachMod) - TODO(currentLocation, "ATTACH modifier is not implemented yet"); mlir::omp::ClauseMapFlags mapTypeBits = mlir::omp::ClauseMapFlags::none; std::string mapperIdName = "__implicit_mapper"; // If the map type is specified, then process it else set the appropriate @@ -1371,6 +1369,20 @@ bool ClauseProcessor::processMap( } } + if (attachMod) { + switch (*attachMod) { + case Map::AttachModifier::Always: + mapTypeBits |= mlir::omp::ClauseMapFlags::attach_always; + break; + case Map::AttachModifier::Never: + mapTypeBits |= mlir::omp::ClauseMapFlags::attach_never; + break; + case Map::AttachModifier::Auto: + mapTypeBits |= mlir::omp::ClauseMapFlags::attach_auto; + break; + } + } + if (iterator) { TODO(currentLocation, "Support for iterator modifiers is not implemented yet"); diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp index acf64c40c9e20..7b11641eb49df 100644 --- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp +++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp @@ -624,11 +624,12 @@ class MapInfoFinalizationPass /// issues. mlir::omp::ClauseMapFlags getDescriptorMapType(mlir::omp::ClauseMapFlags mapTypeFlag, - mlir::Operation *target, bool isHasDeviceAddr) { + mlir::Operation *target, bool isHasDeviceAddr, + bool isAttachNever = false) { using mapFlags = mlir::omp::ClauseMapFlags; mapFlags flags = mapFlags::none; - if (!isHasDeviceAddr) + if (!isHasDeviceAddr && !isAttachNever) flags |= mapFlags::attach; if (llvm::isa_and_nonnullgetLoc(), descMapOp.getResult().getType(), descMapOp.getVarPtr(), descMapOp.getVarTypeAttr(), builder.getAttr( - mlir::omp::ClauseMapFlags::attach | refFlagType), + mlir::omp::ClauseMapFlags::attach | refFlagType | + (isAttachAlways ? mlir::omp::ClauseMapFlags::always + : mlir::omp::ClauseMapFlags::none)), descMapOp.getMapCaptureTypeAttr(), /*varPtrPtr=*/ fir::BoxOffsetOp::create(builder, descMapOp->getLoc(), descriptor, fir::BoxFieldAttr::base_addr), @@ -831,6 +836,13 @@ class MapInfoFinalizationPass // TODO: map the addendum segment of the descriptor, similarly to the // base address/data pointer member. bool isHasDeviceAddrFlag = isHasDeviceAddr(op, *target); + bool isAttachNever = + (op.getMapType() & mlir::omp::ClauseMapFlags::attach_never) == + mlir::omp::ClauseMapFlags::attach_never; + bool isAttachAlways = + (op.getMapType() & mlir::omp::ClauseMapFlags::attach_always) == + mlir::omp::ClauseMapFlags::attach_always; + mlir::Value descriptor = getDescriptorFromBoxMap(op, builder, descCanBeDeferred); if ((op.getMapType() & mlir::omp::ClauseMapFlags::ref_ptr) == @@ -858,43 +870,63 @@ class MapInfoFinalizationPass // feasible. // TODO: Extend this to perhaps check for target updates and target data // with release and from applied. - if (!llvm::isa(target)) - genImplicitAttachMap(op, descriptor, target, builder, - mlir::omp::ClauseMapFlags::ref_ptr); - op.replaceAllUsesWith(newMapInfoOp.getResult()); - op->erase(); + if (!llvm::isa(target) && !isAttachNever) + genImplicitAttachMap(op, descriptor, target, builder, + mlir::omp::ClauseMapFlags::ref_ptr, + isAttachAlways); + op.replaceAllUsesWith(newMapInfoOp.getResult()); + op->erase(); } else if ((op.getMapType() & mlir::omp::ClauseMapFlags::ref_ptee) == mlir::omp::ClauseMapFlags::ref_ptee) { // TODO/FIX: - // 0) i) create two variations of the test with ref_ptr/ptee shuffled around - // ii) test ref_ptr_ptee - // iii) test some other ref_ptr/ptee examples (for example the stack descriptor getting stuck in memory example), and perhaps ask michael if he has any ideas - // - ask Michael what he thinks about turning off the descriptor deferral stuff for newer OpenMP versions by default - // and having to provide a flag in those cases. Just so we can nudge people towards using ref_ptr/ptee in those - // cases (and so we can test the ref_ptr/ptee test trivially) - Might not really matter for downstream as we have - // a flag to toggle it, but I don't think upstream has it - // iiiii) Make sure it works for all existing map tests and check-offload, check-smoke without breakages - // iiiiii) Add these tests to check-smoke with a script that will verify the runtime trace and check attaches are happening at the right time - // as well as transfers of the various bits and pieces... - // 1) Look into the possibility of teaching the backend to only require ATTACH map to do the more - // unique mapping of accessing varPtr/varPtrPtr fields for basePointer/offloadPointer as opposed - // to using the ref_ptee/ref_ptr types... - // 2) Remove the always map on the attach in lowering and try to find/fix the problem case so it's no + // 0) ii) test ref_ptr_ptee + // iii) test some other ref_ptr/ptee examples (for example the stack + // descriptor getting stuck in memory example), and perhaps ask michael + // if he has any ideas iiiii) Make sure it works for all existing map + // tests and check-offload, check-smoke without breakages iiiiii) Add + // these tests to check-smoke with a script that will verify the + // runtime trace and check attaches are happening at the right time as + // well as transfers of the various bits and pieces... + // 1) Look into the possibility of teaching the backend to only require + // ATTACH map to do the more + // unique mapping of accessing varPtr/varPtrPtr fields for + // basePointer/offloadPointer as opposed to using the ref_ptee/ref_ptr + // types... + // 2) Remove the always map on the attach in lowering and try to find/fix + // the problem case so it's no // longer required... // 3) Implement attach always/auto/none - // - we'll want to apply ALWAYS when map always is given, and never apply when (perhaps in subsequent patch so as not to overload everything at once) attach none is provided, but the default case we will just generate this map. - // 4) Test and fix if neccessary the derived type case for ref_ptr/ptee where we apply it to multiple nested - // allocatables, the usual stuff needed to check and fix for descriptor derived type related stuff + // - we'll want to apply ALWAYS when map always is given, and never + // apply when (perhaps in subsequent patch so as not to overload + // everything at once) attach none is provided, but the default case we + // will just generate this map. + // 4) Test and fix if neccessary the derived type case for ref_ptr/ptee + // where we apply it to multiple nested + // allocatables, the usual stuff needed to check and fix for descriptor + // derived type related stuff // 5) look at tidying everything up more. - // 6) - Can we simplify the attach map AND the way we bind the descriptors by using ref_ptr_ptee to indicate - // we're a binding map, or would this cause issues? Or perhaps there's a better way to simplify the backend - // mappings by splitting them into 3 seperate maps in the else clause below, but it might cause issues for derived types - // need to test first.... - // 7) - If 6 works then re-viist if we need attach maps on exit directives for the main combined map, it may have been tied to the fact the lowering + // 6) - Can we simplify the attach map AND the way we bind the + // descriptors by using ref_ptr_ptee to indicate + // we're a binding map, or would this cause issues? Or perhaps there's a + // better way to simplify the backend mappings by splitting them into 3 + // seperate maps in the else clause below, but it might cause issues for + // derived types need to test first.... + // 7) - If 6 works then re-viist if we need attach maps on exit directives + // for the main combined map, it may have been tied to the fact the + // lowering // found itn eccessary - // 8) Can perhaps revisit if we can detach the need for adding base_addresses to the members_of, but might be better to leave that - // to the future for a while as it's more of a tidying up scenario than a neccessity. + // 8) Can perhaps revisit if we can detach the need for adding + // base_addresses to the members_of, but might be better to leave that + // to the future for a while as it's more of a tidying up scenario + // than a neccessity. + // 9) ask Michael what he thinks about turning off the descriptor deferral + // stuff for newer OpenMP versions by default + // and having to provide a flag in those cases. Just so we can nudge + // people towards using ref_ptr/ptee in those cases (and so we can test + // the ref_ptr/ptee test trivially) - Might not really matter for + // downstream as we have a flag to toggle it, but I don't think upstream + // has it // For ref_ptee, we generate a map of the base address with user specified map types and // in the default auto attach case, we generate an additional attach map which indicates @@ -911,12 +943,16 @@ class MapInfoFinalizationPass // just ideal to remove the noise where feasible. // TODO: Extend this to perhaps check for target updates and target data // with release and from applied. - if (!llvm::isa(target)) - genImplicitAttachMap(op, descriptor, target, builder, - mlir::omp::ClauseMapFlags::ref_ptee); - op.replaceAllUsesWith(newMapInfoOp.getResult()); - op->erase(); + if (!llvm::isa(target) && !isAttachNever) + genImplicitAttachMap(op, descriptor, target, builder, + mlir::omp::ClauseMapFlags::ref_ptee, + isAttachAlways); + op.replaceAllUsesWith(newMapInfoOp.getResult()); + op->erase(); } else { + + // TODO: Split this into multiple mappings like above, instead of a parent + // member mapping to try and simplify this and later lowering. bool isRefPtrPtee = (op.getMapType() & mlir::omp::ClauseMapFlags::ref_ptr_ptee) == mlir::omp::ClauseMapFlags::ref_ptr_ptee; @@ -939,11 +975,19 @@ class MapInfoFinalizationPass // If we have been provided RefPtrPtee, utilise the user specified map // types as best we can only providing the additional map types necessary, // otherwise, use the default descriptor map type. + + // NOTE: For the moment isAttachAlways, for this case is handled by not + // removing attach_always, and letting the later lowering handle it, the + // regular always map type isn't equivalent to attach_always at this + // level. At least yet, I have plans to refactor this all in a subsequent + // commit. auto mapType = - isRefPtrPtee ? (op.getMapType() | mlir::omp::ClauseMapFlags::attach | - mlir::omp::ClauseMapFlags::descriptor) - : getDescriptorMapType(op.getMapType(), target, - isHasDeviceAddrFlag); + isRefPtrPtee + ? (op.getMapType() | mlir::omp::ClauseMapFlags::descriptor | + (isAttachNever ? mlir::omp::ClauseMapFlags::none + : mlir::omp::ClauseMapFlags::attach)) + : getDescriptorMapType(op.getMapType(), target, + isHasDeviceAddrFlag, isAttachNever); auto newMapInfoOp = mlir::omp::MapInfoOp::create( builder, op->getLoc(), op.getResult().getType(), descriptor, mlir::TypeAttr::get(fir::unwrapRefType(descriptor.getType())), diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPEnums.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPEnums.td index ada3a3edd8a30..f4b0c6ecb4733 100644 --- a/mlir/include/mlir/Dialect/OpenMP/OpenMPEnums.td +++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPEnums.td @@ -123,7 +123,7 @@ def ClauseMapFlagsPresent : I32BitEnumAttrCaseBit<"present", 10>; def ClauseMapFlagsOMPXHold : I32BitEnumAttrCaseBit<"ompx_hold", 11>; def ClauseMapFlagsAttach : I32BitEnumAttrCaseBit<"attach", 12>; def ClauseMapFlagsAttachAlways : I32BitEnumAttrCaseBit<"attach_always", 13>; -def ClauseMapFlagsAttachNone : I32BitEnumAttrCaseBit<"attach_none", 14>; +def ClauseMapFlagsAttachNever : I32BitEnumAttrCaseBit<"attach_never", 14>; def ClauseMapFlagsAttachAuto : I32BitEnumAttrCaseBit<"attach_auto", 15>; def ClauseMapFlagsRefPtr : I32BitEnumAttrCaseBit<"ref_ptr", 16>; def ClauseMapFlagsRefPtee : I32BitEnumAttrCaseBit<"ref_ptee", 17>; @@ -148,7 +148,7 @@ def ClauseMapFlags : OpenMP_BitEnumAttr< ClauseMapFlagsOMPXHold, ClauseMapFlagsAttach, ClauseMapFlagsAttachAlways, - ClauseMapFlagsAttachNone, + ClauseMapFlagsAttachNever, ClauseMapFlagsAttachAuto, ClauseMapFlagsRefPtr, ClauseMapFlagsRefPtee, diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp index 172f21ff1779e..d5c6f4e15462c 100644 --- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp +++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp @@ -1806,8 +1806,8 @@ static ParseResult parseMapClause(OpAsmParser &parser, if (mapTypeMod == "attach_always") mapTypeBits |= ClauseMapFlags::attach_always; - if (mapTypeMod == "attach_none") - mapTypeBits |= ClauseMapFlags::attach_none; + if (mapTypeMod == "attach_never") + mapTypeBits |= ClauseMapFlags::attach_never; if (mapTypeMod == "attach_auto") mapTypeBits |= ClauseMapFlags::attach_auto; @@ -1882,8 +1882,8 @@ static void printMapClause(OpAsmPrinter &p, Operation *op, mapTypeStrs.push_back("attach"); if (mapTypeToBool(mapFlags, ClauseMapFlags::attach_always)) mapTypeStrs.push_back("attach_always"); - if (mapTypeToBool(mapFlags, ClauseMapFlags::attach_none)) - mapTypeStrs.push_back("attach_none"); + if (mapTypeToBool(mapFlags, ClauseMapFlags::attach_never)) + mapTypeStrs.push_back("attach_never"); if (mapTypeToBool(mapFlags, ClauseMapFlags::attach_auto)) mapTypeStrs.push_back("attach_auto"); if (mapTypeToBool(mapFlags, ClauseMapFlags::ref_ptr)) diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp index c310b1c7dc1dd..ed2321ad6426d 100644 --- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp +++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp @@ -4488,14 +4488,13 @@ gatherImmediateMembers(llvm::SmallVector &immediateMapDataIdxs, } } -void processAttachMap(LLVM::ModuleTranslation &moduleTranslation, - llvm::IRBuilderBase &builder, - llvm::OpenMPIRBuilder &ompBuilder, DataLayout &dl, - MapInfosTy &combinedInfo, MapInfoData &mapData, - uint64_t mapDataIndex, bool parentMap, - llvm::SmallVectorImpl &immediateMapDataIdxs, - llvm::omp::OpenMPOffloadMappingFlags memberOfFlag, - TargetDirective targetDirective) { +static void processAttachMap( + LLVM::ModuleTranslation &moduleTranslation, llvm::IRBuilderBase &builder, + llvm::OpenMPIRBuilder &ompBuilder, DataLayout &dl, MapInfosTy &combinedInfo, + MapInfoData &mapData, uint64_t mapDataIndex, bool parentMap, + llvm::SmallVectorImpl &immediateMapDataIdxs, + llvm::omp::OpenMPOffloadMappingFlags memberOfFlag, + TargetDirective targetDirective, bool alwaysAttach = false) { assert(!ompBuilder.Config.isTargetDevice() && "function only supported for host device codegen"); auto parentClause = @@ -4576,8 +4575,9 @@ void processAttachMap(LLVM::ModuleTranslation &moduleTranslation, // update it we can run into some unusual issues where the attachment is // still blocked. combinedInfo.Types.emplace_back( - llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ATTACH/* | - llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS*/); + llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ATTACH | + ((alwaysAttach) ? llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS + : llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE)); combinedInfo.DevicePointers.emplace_back( llvm::OpenMPIRBuilder::DeviceInfoTy::None); combinedInfo.Mappers.emplace_back(mapData.Mappers[mapDataIndex]); @@ -4841,9 +4841,15 @@ static void processMapWithMembersOf(LLVM::ModuleTranslation &moduleTranslation, llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ATTACH) { llvm::SmallVector immediateMapDataIdxs; gatherImmediateMembers(immediateMapDataIdxs, mapData, parentClause, map); - processAttachMap(moduleTranslation, builder, ompBuilder, dl, combinedInfo, - mapData, mapInfoIdx, i == 0, immediateMapDataIdxs, - memberOfFlag, targetDirective); + // TODO: Handle attach_always more elegantly in the frontend, we will have + // to split up the ref_ptr_ptee/default Fortran pointer and allocatable + // mapping into their constituient components to do so. + processAttachMap( + moduleTranslation, builder, ompBuilder, dl, combinedInfo, mapData, + mapInfoIdx, i == 0, immediateMapDataIdxs, memberOfFlag, + targetDirective, + checkHasClauseMapFlag(map.getMapType(), + omp::ClauseMapFlags::attach_always)); i++; continue; } From 70d77921cf3772a806907c4b12a3b5ebe61dae47 Mon Sep 17 00:00:00 2001 From: agozillon Date: Wed, 12 Nov 2025 23:51:54 -0600 Subject: [PATCH 7/8] Revert "[Flang] Fix defaultmap(none) being overly aggressive with symbol checks" This reverts commit 64092a7d1f45eeae65f733bf017b0b70b42ce264. --- flang/lib/Semantics/resolve-directives.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp index 939e8960d454c..3744e43e98a28 100644 --- a/flang/lib/Semantics/resolve-directives.cpp +++ b/flang/lib/Semantics/resolve-directives.cpp @@ -3072,8 +3072,7 @@ void OmpAttributeVisitor::Post(const parser::Name &name) { if (Symbol * found{currScope().FindSymbol(name.source)}) { // If the variable has declare target applied to it (enter or link) it // is exempt from defaultmap(none) restrictions - if (!symbol->GetUltimate().test(Symbol::Flag::OmpDeclareTarget) && - !IsProcedure(*symbol) && !IsNamedConstant(*symbol)) { + if (!symbol->GetUltimate().test(Symbol::Flag::OmpDeclareTarget)) { auto &dMap = GetContext().defaultMap; for (auto defaults : dMap) { if (defaults.second == From e0e92cdf426c0372c0773287315497d483b96a39 Mon Sep 17 00:00:00 2001 From: agozillon Date: Thu, 20 Nov 2025 02:31:23 -0600 Subject: [PATCH 8/8] WIP ref_ptr_ptee attach clause work Fixes most of the local tests I have after moving the attach work for ref_ptr_ptee cases to the same location as ref_ptr/ptee, except 1... --- .../Optimizer/OpenMP/AutomapToTargetData.cpp | 1 + .../Optimizer/OpenMP/LowerWorkdistribute.cpp | 1 + .../Optimizer/OpenMP/MapInfoFinalization.cpp | 319 +++++++++++++----- .../OpenMP/MapsForPrivatizedSymbols.cpp | 2 +- flang/lib/Utils/OpenMP.cpp | 29 +- mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | 11 +- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | 25 +- .../OpenMPOffloadPrivatizationPrepare.cpp | 2 +- .../OpenMP/OpenMPToLLVMIRTranslation.cpp | 220 +++++++----- 9 files changed, 416 insertions(+), 194 deletions(-) diff --git a/flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp b/flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp index 5793d46a192a7..eeb08ebf51191 100644 --- a/flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp +++ b/flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp @@ -124,6 +124,7 @@ class AutomapToTargetDataPass builder.getAttr( omp::VariableCaptureKind::ByCopy), /*var_ptr_ptr=*/mlir::Value{}, + /*var_ptr_ptr_type=*/mlir::TypeAttr{}, /*members=*/SmallVector{}, /*members_index=*/ArrayAttr{}, bounds, /*mapperId=*/mlir::FlatSymbolRefAttr(), globalOp.getSymNameAttr(), diff --git a/flang/lib/Optimizer/OpenMP/LowerWorkdistribute.cpp b/flang/lib/Optimizer/OpenMP/LowerWorkdistribute.cpp index 7b61539984232..d877d850f6d82 100644 --- a/flang/lib/Optimizer/OpenMP/LowerWorkdistribute.cpp +++ b/flang/lib/Optimizer/OpenMP/LowerWorkdistribute.cpp @@ -839,6 +839,7 @@ static TempOmpVar allocateTempOmpVar(Location loc, Type ty, rewriter.getAttr( omp::VariableCaptureKind::ByRef), /*varPtrPtr=*/Value{}, + /*varPtrPtrType=*/mlir::TypeAttr{}, /*members=*/SmallVector{}, /*member_index=*/mlir::ArrayAttr{}, /*bounds=*/ValueRange(), diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp index 7b11641eb49df..64695db38c136 100644 --- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp +++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp @@ -200,7 +200,7 @@ class MapInfoFinalizationPass op.getMapTypeAttr(), builder.getAttr( mlir::omp::VariableCaptureKind::ByRef), - /*varPtrPtr=*/mlir::Value{}, /*members=*/mlir::ValueRange{}, + /*varPtrPtr=*/mlir::Value{}, /*varPtrPtr=*/mlir::TypeAttr{}, /*members=*/mlir::ValueRange{}, /*members_index=*/mlir::ArrayAttr{}, bounds, /*mapperId=*/mlir::FlatSymbolRefAttr(), builder.getStringAttr(op.getNameAttr().strref() + "." + memberName + @@ -308,7 +308,7 @@ class MapInfoFinalizationPass fir::FirOpBuilder &builder, bool &canDescBeDeferred) { mlir::Value descriptor = boxMap.getVarPtr(); - if (!fir::isTypeWithDescriptor(boxMap.getVarType())) + if (!fir::isTypeWithDescriptor(boxMap.getVarPtrType())) if (auto addrOp = mlir::dyn_cast_if_present( boxMap.getVarPtr().getDefiningOp())) descriptor = addrOp.getVal(); @@ -377,22 +377,26 @@ class MapInfoFinalizationPass mlir::Value baseAddrAddr = fir::BoxOffsetOp::create( builder, loc, descriptor, fir::BoxFieldAttr::base_addr); - mlir::Type underlyingVarType = + mlir::Type underlyingBaseAddrType = llvm::cast( fir::unwrapRefType(baseAddrAddr.getType())) .getElementType(); - if (auto seqType = llvm::dyn_cast(underlyingVarType)) + if (auto seqType = + llvm::dyn_cast(underlyingBaseAddrType)) if (seqType.hasDynamicExtents()) - underlyingVarType = seqType.getEleTy(); + underlyingBaseAddrType = seqType.getEleTy(); + + mlir::Type underlyingDescType = fir::unwrapRefType(descriptor.getType()); // Member of the descriptor pointing at the allocated data return mlir::omp::MapInfoOp::create( builder, loc, baseAddrAddr.getType(), descriptor, - mlir::TypeAttr::get(underlyingVarType), + mlir::TypeAttr::get(underlyingDescType), builder.getAttr(mapType), builder.getAttr( mlir::omp::VariableCaptureKind::ByRef), - baseAddrAddr, /*members=*/mlir::SmallVector{}, + baseAddrAddr, mlir::TypeAttr::get(underlyingBaseAddrType), + /*members=*/mlir::SmallVector{}, /*membersIndex=*/mlir::ArrayAttr{}, bounds, /*mapperId*/ mlir::FlatSymbolRefAttr(), /*name=*/builder.getStringAttr(""), @@ -443,6 +447,85 @@ class MapInfoFinalizationPass baseAddrIndex); } + // This functions aims to insert new maps derived from existing maps into the + // corresponding clause list, interlinking it correctly with block arguments + // where required . + void addDerivedMemberToTarget(mlir::omp::MapInfoOp owner, + mlir::omp::MapInfoOp derived, + llvm::SmallVectorImpl &mapMemberUsers, + fir::FirOpBuilder &builder, + mlir::Operation *target) { + auto addOperands = [&](mlir::MutableOperandRange &mapVarsArr, + mlir::Operation *directiveOp, + unsigned blockArgInsertIndex = 0) { + // Check we're inserting into the correct MapInfoOp list + if (!llvm::is_contained(mapVarsArr.getAsOperandRange(), + mapMemberUsers.empty() + ? owner.getResult() + : mapMemberUsers[0].parent.getResult())) + return; + + // Check we're not inserting a duplicate map. + if (llvm::is_contained(mapVarsArr.getAsOperandRange(), + derived.getResult())) + return; + + // There doesn't appear to be a simple way to convert MutableOperandRange + // to a vector currently, so we instead use a for_each to populate our + // vector. + llvm::SmallVector newMapOps; + newMapOps.reserve(mapVarsArr.size()); + llvm::for_each( + mapVarsArr.getAsOperandRange(), + [&newMapOps](mlir::Value oper) { newMapOps.push_back(oper); }); + + newMapOps.push_back(derived); + if (directiveOp) { + directiveOp->getRegion(0).insertArgument( + blockArgInsertIndex, derived.getType(), derived.getLoc()); + blockArgInsertIndex++; + } + + mapVarsArr.assign(newMapOps); + }; + + auto argIface = + llvm::dyn_cast(target); + + if (auto mapClauseOwner = + llvm::dyn_cast(target)) { + mlir::MutableOperandRange mapVarsArr = mapClauseOwner.getMapVarsMutable(); + unsigned blockArgInsertIndex = + argIface + ? argIface.getMapBlockArgsStart() + argIface.numMapBlockArgs() + : 0; + addOperands(mapVarsArr, + llvm::dyn_cast_if_present( + argIface.getOperation()), + blockArgInsertIndex); + } + + if (auto targetDataOp = llvm::dyn_cast(target)) { + mlir::MutableOperandRange useDevAddrMutableOpRange = + targetDataOp.getUseDeviceAddrVarsMutable(); + addOperands(useDevAddrMutableOpRange, target, + argIface.getUseDeviceAddrBlockArgsStart() + + argIface.numUseDeviceAddrBlockArgs()); + + mlir::MutableOperandRange useDevPtrMutableOpRange = + targetDataOp.getUseDevicePtrVarsMutable(); + addOperands(useDevPtrMutableOpRange, target, + argIface.getUseDevicePtrBlockArgsStart() + + argIface.numUseDevicePtrBlockArgs()); + } else if (auto targetOp = llvm::dyn_cast(target)) { + mlir::MutableOperandRange hasDevAddrMutableOpRange = + targetOp.getHasDeviceAddrVarsMutable(); + addOperands(hasDevAddrMutableOpRange, target, + argIface.getHasDeviceAddrBlockArgsStart() + + argIface.numHasDeviceAddrBlockArgs()); + } + } + // We add all mapped record members not directly used in the target region // to the block arguments in front of their parent and we place them into // the map operands list for consistency. @@ -482,14 +565,12 @@ class MapInfoFinalizationPass // specification to map these (and any recursive components) in their // entirety, which is different to the C++ equivalent, which requires // explicit mapping of these segments. - void addImplicitMembersToTarget(mlir::omp::MapInfoOp op, - fir::FirOpBuilder &builder, - mlir::Operation *target, - bool insertMapOp = false) { +void addImplicitMembersToTarget(mlir::omp::MapInfoOp op, + fir::FirOpBuilder &builder, + mlir::Operation *target) { auto mapClauseOwner = llvm::dyn_cast_if_present( target); - // TargetDataOp is technically a MapClauseOwningOpInterface, so we // do not need to explicitly check for the extra cases here for use_device // addr/ptr @@ -499,38 +580,27 @@ class MapInfoFinalizationPass auto addOperands = [&](mlir::MutableOperandRange &mapVarsArr, mlir::Operation *directiveOp, unsigned blockArgInsertIndex = 0) { - if (!llvm::is_contained(mapVarsArr.getAsOperandRange(), op.getResult()) && - !insertMapOp) + if (!llvm::is_contained(mapVarsArr.getAsOperandRange(), op.getResult())) return; - llvm::SmallVector newMapOps; - auto insertMap = [&](mlir::Value newInsert) { - newMapOps.push_back(newInsert); - if (directiveOp) { - directiveOp->getRegion(0).insertArgument( - blockArgInsertIndex, newInsert.getType(), newInsert.getLoc()); - blockArgInsertIndex++; - } - }; - // There doesn't appear to be a simple way to convert MutableOperandRange // to a vector currently, so we instead use a for_each to populate our // vector. + llvm::SmallVector newMapOps; newMapOps.reserve(mapVarsArr.size()); llvm::for_each( mapVarsArr.getAsOperandRange(), [&newMapOps](mlir::Value oper) { newMapOps.push_back(oper); }); - // if the map doesn't exist on the target already, then we add it. - if (!llvm::is_contained(mapVarsArr.getAsOperandRange(), op.getResult())) - insertMap(op); - - // Insert all of the members that the map contains into the new target map - // list. for (auto mapMember : op.getMembers()) { if (llvm::is_contained(mapVarsArr.getAsOperandRange(), mapMember)) continue; - insertMap(mapMember); + newMapOps.push_back(mapMember); + if (directiveOp) { + directiveOp->getRegion(0).insertArgument( + blockArgInsertIndex, mapMember.getType(), mapMember.getLoc()); + blockArgInsertIndex++; + } } mapVarsArr.assign(newMapOps); @@ -623,6 +693,26 @@ class MapInfoFinalizationPass /// and some of the runtime descriptor behaviour at the moment can cause /// issues. mlir::omp::ClauseMapFlags + getDescriptorMapType(mlir::omp::ClauseMapFlags mapTypeFlag, + mlir::Operation *target) { + using mapFlags = mlir::omp::ClauseMapFlags; + mapFlags flags = mapFlags::none; + + if (llvm::isa_and_nonnull(target)) { + flags |= mapTypeFlag | mapFlags::descriptor; + return flags; + } + + flags |= mapFlags::to | mapFlags::descriptor | mapFlags::always | + (mapTypeFlag & mapFlags::implicit); + + if (moduleRequiresUSM(target->getParentOfType())) + flags |= mapFlags::close; + return flags; + } + + mlir::omp::ClauseMapFlags getDescriptorMapType(mlir::omp::ClauseMapFlags mapTypeFlag, mlir::Operation *target, bool isHasDeviceAddr, bool isAttachNever = false) { @@ -678,29 +768,58 @@ class MapInfoFinalizationPass return false; } - void genImplicitAttachMap(mlir::omp::MapInfoOp descMapOp, - mlir::Value descriptor, mlir::Operation *target, - fir::FirOpBuilder &builder, - mlir::omp::ClauseMapFlags refFlagType, - bool isAttachAlways = false) { + void genImplicitAttachMap( + mlir::omp::MapInfoOp descMapOp, mlir::Value descriptor, + llvm::SmallVectorImpl &mapMemberUsers, + mlir::Operation *target, fir::FirOpBuilder &builder, + mlir::omp::ClauseMapFlags refFlagType, bool isAttachAlways = false) { + // auto implicitAttachMap = mlir::omp::MapInfoOp::create( + // builder, descMapOp->getLoc(), descMapOp.getResult().getType(), + // descriptor, + // mlir::TypeAttr::get(fir::unwrapRefType(descriptor.getType())), + // builder.getAttr( + // mlir::omp::ClauseMapFlags::attach | refFlagType | + // (isAttachAlways ? mlir::omp::ClauseMapFlags::always + // : mlir::omp::ClauseMapFlags::none)), + // descMapOp.getMapCaptureTypeAttr(), /*varPtrPtr=*/ + // fir::BoxOffsetOp::create(builder, descMapOp->getLoc(), descriptor, + // fir::BoxFieldAttr::base_addr), + // /*members=*/mlir::SmallVector{}, + // /*membersIndex=*/mlir::ArrayAttr{}, + // /*bounds=*/mlir::SmallVector{}, + // /*mapperId*/ mlir::FlatSymbolRefAttr(), descMapOp.getNameAttr(), + // /*partial_map=*/builder.getBoolAttr(false)); + + // AG NOTE: If we keep this make it a helper function + auto baseAddrAddr = fir::BoxOffsetOp::create( + builder, descMapOp->getLoc(), descriptor, fir::BoxFieldAttr::base_addr); + mlir::Type underlyingVarType = + llvm::cast( + fir::unwrapRefType(baseAddrAddr.getType())) + .getElementType(); + if (auto seqType = llvm::dyn_cast(underlyingVarType)) + if (seqType.hasDynamicExtents()) + underlyingVarType = seqType.getEleTy(); + auto implicitAttachMap = mlir::omp::MapInfoOp::create( builder, descMapOp->getLoc(), descMapOp.getResult().getType(), - descMapOp.getVarPtr(), descMapOp.getVarTypeAttr(), + descriptor, mlir::TypeAttr::get(fir::unwrapRefType(descriptor.getType())), builder.getAttr( mlir::omp::ClauseMapFlags::attach | refFlagType | (isAttachAlways ? mlir::omp::ClauseMapFlags::always : mlir::omp::ClauseMapFlags::none)), descMapOp.getMapCaptureTypeAttr(), /*varPtrPtr=*/ - fir::BoxOffsetOp::create(builder, descMapOp->getLoc(), descriptor, - fir::BoxFieldAttr::base_addr), - descMapOp.getMembers(), descMapOp.getMembersIndexAttr(), - /*bounds=*/mlir::SmallVector{}, + baseAddrAddr, mlir::TypeAttr::get(underlyingVarType), + /*members=*/mlir::SmallVector{}, + /*membersIndex=*/mlir::ArrayAttr{}, + /*bounds=*/descMapOp.getBounds(), /*mapperId*/ mlir::FlatSymbolRefAttr(), descMapOp.getNameAttr(), /*partial_map=*/builder.getBoolAttr(false)); // Has to be added to the target immediately, as we expect all maps // processed by this pass to have a user that is a target. - addImplicitMembersToTarget(implicitAttachMap, builder, target, true); + addDerivedMemberToTarget(descMapOp, implicitAttachMap, mapMemberUsers, + builder, target); } // Expand mappings of type(C_PTR) to map their `__address` field explicitly @@ -746,7 +865,7 @@ class MapInfoFinalizationPass mlir::TypeAttr::get(fir::unwrapRefType(coord.getType())), mapTypeAttr, builder.getAttr( mlir::omp::VariableCaptureKind::ByRef), - /*varPtrPtr=*/mlir::Value{}, + /*varPtrPtr=*/mlir::Value{}, mlir::TypeAttr{}, /*members=*/llvm::SmallVector{}, /*member_index=*/mlir::ArrayAttr{}, /*bounds=*/op.getBounds(), @@ -757,8 +876,8 @@ class MapInfoFinalizationPass // Rebuild the parent as a container with the `__address` member. mlir::omp::MapInfoOp newParent = mlir::omp::MapInfoOp::create( builder, op.getLoc(), op.getResult().getType(), op.getVarPtr(), - op.getVarTypeAttr(), mapTypeAttr, op.getMapCaptureTypeAttr(), - /*varPtrPtr=*/mlir::Value{}, + op.getVarPtrTypeAttr(), mapTypeAttr, op.getMapCaptureTypeAttr(), + /*varPtrPtr=*/mlir::Value{}, mlir::TypeAttr{}, /*members=*/llvm::SmallVector{memberMap}, /*member_index=*/newMembersAttr, /*bounds=*/llvm::SmallVector{}, @@ -852,27 +971,28 @@ class MapInfoFinalizationPass // to the runtime to try and attach the base address to the descriptor if it's available // and it's the first time the ref_ptr has been allocated on the device. auto newMapInfoOp = mlir::omp::MapInfoOp::create( - builder, op->getLoc(), op.getResult().getType(), op.getVarPtr(), - op.getVarTypeAttr(), - builder.getAttr( - op.getMapType() & ~mlir::omp::ClauseMapFlags::ref_ptr), - op.getMapCaptureTypeAttr(), /*varPtrPtr=*/op.getVarPtrPtr(), - op.getMembers(), op.getMembersIndexAttr(), - /*bounds=*/mlir::SmallVector{}, - /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), - /*partial_map=*/builder.getBoolAttr(false)); - - // If we're a map exiting construct we skip the generation of the - // attach map, it should be unnecessary in these cases as it exists to - // bind the pointer and pointee and shouldn't increment or decrement - // the ref counter on its own. However, equally having it doesn't - // cause issues either, it's just ideal to remove the noise where - // feasible. - // TODO: Extend this to perhaps check for target updates and target data - // with release and from applied. + builder, op->getLoc(), op.getResult().getType(), op.getVarPtr(), + op.getVarPtrTypeAttr(), + builder.getAttr( + op.getMapType() & ~mlir::omp::ClauseMapFlags::ref_ptr), + op.getMapCaptureTypeAttr(), /*varPtrPtr=*/op.getVarPtrPtr(), + /*varPtrPtrType=*/op.getVarPtrPtrTypeAttr(), op.getMembers(), + op.getMembersIndexAttr(), + /*bounds=*/mlir::SmallVector{}, + /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), + /*partial_map=*/builder.getBoolAttr(false)); + + // If we're a map exiting construct we skip the generation of the + // attach map, it should be unnecessary in these cases as it exists to + // bind the pointer and pointee and shouldn't increment or decrement + // the ref counter on its own. However, equally having it doesn't + // cause issues either, it's just ideal to remove the noise where + // feasible. + // TODO: Extend this to perhaps check for target updates and target data + // with release and from applied. if (!llvm::isa(target) && !isAttachNever) - genImplicitAttachMap(op, descriptor, target, builder, - mlir::omp::ClauseMapFlags::ref_ptr, + genImplicitAttachMap(op, descriptor, mapMemberUsers, target, builder, + mlir::omp::ClauseMapFlags::ref_ptr/*_ptee*/, isAttachAlways); op.replaceAllUsesWith(newMapInfoOp.getResult()); op->erase(); @@ -944,8 +1064,8 @@ class MapInfoFinalizationPass // TODO: Extend this to perhaps check for target updates and target data // with release and from applied. if (!llvm::isa(target) && !isAttachNever) - genImplicitAttachMap(op, descriptor, target, builder, - mlir::omp::ClauseMapFlags::ref_ptee, + genImplicitAttachMap(op, descriptor, mapMemberUsers, target, builder, + mlir::omp::ClauseMapFlags::ref_ptr_ptee, isAttachAlways); op.replaceAllUsesWith(newMapInfoOp.getResult()); op->erase(); @@ -981,26 +1101,41 @@ class MapInfoFinalizationPass // regular always map type isn't equivalent to attach_always at this // level. At least yet, I have plans to refactor this all in a subsequent // commit. - auto mapType = + auto mapType = isRefPtrPtee - ? (op.getMapType() | mlir::omp::ClauseMapFlags::descriptor | - (isAttachNever ? mlir::omp::ClauseMapFlags::none - : mlir::omp::ClauseMapFlags::attach)) - : getDescriptorMapType(op.getMapType(), target, - isHasDeviceAddrFlag, isAttachNever); - auto newMapInfoOp = mlir::omp::MapInfoOp::create( - builder, op->getLoc(), op.getResult().getType(), descriptor, - mlir::TypeAttr::get(fir::unwrapRefType(descriptor.getType())), - builder.getAttr(mapType), - op.getMapCaptureTypeAttr(), /*varPtrPtr=*/mlir::Value{}, newMembers, - newMembersAttr, /*bounds=*/mlir::SmallVector{}, - /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), - /*partial_map=*/builder.getBoolAttr(false)); - op.replaceAllUsesWith(newMapInfoOp.getResult()); - op->erase(); - - if (descCanBeDeferred) - deferrableDesc.push_back(newMapInfoOp); + ? (op.getMapType() | mlir::omp::ClauseMapFlags::descriptor) + : getDescriptorMapType(op.getMapType(), target); + + // auto mapType = + // isRefPtrPtee + // ? (op.getMapType() | mlir::omp::ClauseMapFlags::descriptor | + // (isAttachNever ? mlir::omp::ClauseMapFlags::none + // : mlir::omp::ClauseMapFlags::attach)) + // : getDescriptorMapType(op.getMapType(), target, + // isHasDeviceAddrFlag, isAttachNever); + auto newMapInfoOp = mlir::omp::MapInfoOp::create( + builder, op->getLoc(), op.getResult().getType(), descriptor, + mlir::TypeAttr::get(fir::unwrapRefType(descriptor.getType())), + builder.getAttr(mapType), + op.getMapCaptureTypeAttr(), /*varPtrPtr=*/mlir::Value{}, + /*varPtrPtTyper=*/mlir::TypeAttr{}, newMembers, newMembersAttr, + /*bounds=*/mlir::SmallVector{}, + /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), + /*partial_map=*/builder.getBoolAttr(false)); + + // do we make this the unique case of not having this implicit attach map + // for now...? + // - as there's a lot of broken tests... 171 vs 150... + if (!llvm::isa(target) && !isAttachNever) + genImplicitAttachMap(op, descriptor, mapMemberUsers, target, builder, + mlir::omp::ClauseMapFlags::ref_ptr_ptee, + isAttachAlways); + + op.replaceAllUsesWith(newMapInfoOp.getResult()); + op->erase(); + + if (descCanBeDeferred) + deferrableDesc.push_back(newMapInfoOp); } } @@ -1046,12 +1181,13 @@ class MapInfoFinalizationPass mlir::omp::MapInfoOp newDescParentMapOp = mlir::omp::MapInfoOp::create( builder, op->getLoc(), op.getResult().getType(), op.getVarPtr(), - op.getVarTypeAttr(), + op.getVarPtrTypeAttr(), builder.getAttr( mlir::omp::ClauseMapFlags::to | mlir::omp::ClauseMapFlags::always | mlir::omp::ClauseMapFlags::descriptor), op.getMapCaptureTypeAttr(), /*varPtrPtr=*/mlir::Value{}, - mlir::SmallVector{}, mlir::ArrayAttr{}, + /*varPtrPtrType=*/mlir::TypeAttr{}, mlir::SmallVector{}, + mlir::ArrayAttr{}, /*bounds=*/mlir::SmallVector{}, /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), /*partial_map=*/builder.getBoolAttr(false)); @@ -1108,8 +1244,9 @@ class MapInfoFinalizationPass builder.loadIfRef(op->getLoc(), baseAddr.getVarPtrPtr()); mlir::omp::MapInfoOp newBaseAddrMapOp = mlir::omp::MapInfoOp::create( builder, op->getLoc(), loadBaseAddr.getType(), loadBaseAddr, - baseAddr.getVarTypeAttr(), baseAddr.getMapTypeAttr(), - baseAddr.getMapCaptureTypeAttr(), mlir::Value{}, members, membersAttr, + baseAddr.getVarPtrTypeAttr(), baseAddr.getMapTypeAttr(), + baseAddr.getMapCaptureTypeAttr(), /*varPtrPtr=*/mlir::Value{}, + /*varPtrPtrType=*/mlir::TypeAttr{}, members, membersAttr, baseAddr.getBounds(), /*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(), /*partial_map=*/builder.getBoolAttr(false)); @@ -1376,7 +1513,7 @@ class MapInfoFinalizationPass "of a MapInfoOp"); if (hasADescriptor(op.getVarPtr().getDefiningOp(), - fir::unwrapRefType(op.getVarType()))) { + fir::unwrapRefType(op.getVarPtrType()))) { builder.setInsertionPoint(op); mlir::Operation *targetUser = getFirstTargetUser(op); assert(targetUser && "expected user of map operation was not found"); diff --git a/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp b/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp index 1d10c5b8dec41..70f36b12d4330 100644 --- a/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp +++ b/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp @@ -132,7 +132,7 @@ class MapsForPrivatizedSymbolsPass llvm::cast(varType).getElementType()), builder.getAttr(mapFlag), builder.getAttr(captureKind), - /*varPtrPtr=*/Value{}, + /*varPtrPtr=*/Value{}, /*varPtrPtrType=*/TypeAttr{}, /*members=*/SmallVector{}, /*member_index=*/mlir::ArrayAttr{}, /*bounds=*/boundsOps, diff --git a/flang/lib/Utils/OpenMP.cpp b/flang/lib/Utils/OpenMP.cpp index b07caf853191a..50fc7b5ab3cf8 100644 --- a/flang/lib/Utils/OpenMP.cpp +++ b/flang/lib/Utils/OpenMP.cpp @@ -26,26 +26,31 @@ mlir::omp::MapInfoOp createMapInfoOp(mlir::OpBuilder &builder, mlir::omp::VariableCaptureKind mapCaptureType, mlir::Type retTy, bool partialMap, mlir::FlatSymbolRefAttr mapperId) { + auto getPtrVarType = [](mlir::Type ptrType) { + mlir::TypeAttr varType = mlir::TypeAttr::get( + llvm::cast(ptrType).getElementType()); + + // For types with unknown extents such as <2x?xi32> we discard the + // incomplete type info and only retain the base type. The correct + // dimensions are later recovered through the bounds info. + if (auto seqType = llvm::dyn_cast(varType.getValue())) + if (seqType.hasDynamicExtents()) + varType = mlir::TypeAttr::get(seqType.getEleTy()); + return varType; + }; + if (auto boxTy = llvm::dyn_cast(baseAddr.getType())) { baseAddr = fir::BoxAddrOp::create(builder, loc, baseAddr); retTy = baseAddr.getType(); } - mlir::TypeAttr varType = mlir::TypeAttr::get( - llvm::cast(retTy).getElementType()); - - // For types with unknown extents such as <2x?xi32> we discard the incomplete - // type info and only retain the base type. The correct dimensions are later - // recovered through the bounds info. - if (auto seqType = llvm::dyn_cast(varType.getValue())) - if (seqType.hasDynamicExtents()) - varType = mlir::TypeAttr::get(seqType.getEleTy()); - + auto varPtrType = getPtrVarType(retTy); + auto varPtrPtrTy = varPtrPtr ? getPtrVarType(varPtrPtr.getType()) : mlir::TypeAttr{}; mlir::omp::MapInfoOp op = - mlir::omp::MapInfoOp::create(builder, loc, retTy, baseAddr, varType, + mlir::omp::MapInfoOp::create(builder, loc, retTy, baseAddr, varPtrType, builder.getAttr(mapType), builder.getAttr(mapCaptureType), - varPtrPtr, members, membersIndex, bounds, mapperId, + varPtrPtr, varPtrPtrTy, members, membersIndex, bounds, mapperId, builder.getStringAttr(name), builder.getBoolAttr(partialMap)); return op; } diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td index 377f1febf6b8f..292e5da465c86 100644 --- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td +++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td @@ -1197,10 +1197,11 @@ def MapBoundsOp : OpenMP_Op<"map.bounds", def MapInfoOp : OpenMP_Op<"map.info", [AttrSizedOperandSegments]> { let arguments = (ins OpenMP_PointerLikeType:$var_ptr, - TypeAttr:$var_type, + TypeAttr:$var_ptr_type, ClauseMapFlagsAttr:$map_type, VariableCaptureKindAttr:$map_capture_type, Optional:$var_ptr_ptr, + OptionalAttr:$var_ptr_ptr_type, Variadic:$members, OptionalAttr:$members_index, Variadic:$bounds, /* rank-0 to rank-{n-1} */ @@ -1239,7 +1240,7 @@ def MapInfoOp : OpenMP_Op<"map.info", [AttrSizedOperandSegments]> { Description of arguments: - `var_ptr`: The address of variable to copy. - - `var_type`: The type of the variable to copy. + - `var_ptr_type`: The type of the var_ptr variable to copy. - 'map_type': OpenMP map type for this map capture, for example: from, to and always. It's a bitfield composed of the OpenMP runtime flags stored in OpenMPOffloadMappingFlags. @@ -1247,6 +1248,8 @@ def MapInfoOp : OpenMP_Op<"map.info", [AttrSizedOperandSegments]> { this can affect how the variable is lowered. - `var_ptr_ptr`: Used when the variable copied is a member of a class, structure or derived type and refers to the originating struct. + - `var_ptr_ptr_type`: Used when the variable copied is a member of a class, structure + or derived type and refers to the originating struct type. - `members`: Used to indicate mapped child members for the current MapInfoOp, represented as other MapInfoOp's, utilised in cases where a parent structure type and members of the structure type are being mapped at the same time. @@ -1267,11 +1270,11 @@ def MapInfoOp : OpenMP_Op<"map.info", [AttrSizedOperandSegments]> { }]; let assemblyFormat = [{ - `var_ptr` `(` $var_ptr `:` type($var_ptr) `,` $var_type `)` + `var_ptr` `(` $var_ptr `:` type($var_ptr) `,` $var_ptr_type `)` `map_clauses` `(` custom($map_type) `)` `capture` `(` custom($map_capture_type) `)` oilist( - `var_ptr_ptr` `(` $var_ptr_ptr `:` type($var_ptr_ptr) `)` + `var_ptr_ptr` `(` $var_ptr_ptr `:` type($var_ptr_ptr) `,` $var_ptr_ptr_type`)` | `mapper` `(` $mapper_id `)` | `members` `(` $members `:` custom($members_index) `:` type($members) `)` | `bounds` `(` $bounds `)` diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp index d5c6f4e15462c..93e0bfeb01a5e 100644 --- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp +++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp @@ -2007,6 +2007,8 @@ static LogicalResult verifyMapClause(Operation *op, OperandRange mapVars) { bool close = mapTypeToBool(mapTypeBits, ClauseMapFlags::close); bool implicit = mapTypeToBool(mapTypeBits, ClauseMapFlags::implicit); + bool attach = mapTypeToBool(mapTypeBits, ClauseMapFlags::attach); + if ((isa(op) || isa(op)) && del) return emitError(op->getLoc(), "to, from, tofrom and alloc map types are permitted"); @@ -2025,10 +2027,11 @@ static LogicalResult verifyMapClause(Operation *op, OperandRange mapVars) { "specified, other map types are not permitted"); } - if (!to && !from) { - return emitError(op->getLoc(), - "at least one of to or from map types must be " - "specified, other map types are not permitted"); + if (!to && !from && !attach) { + return emitError( + op->getLoc(), + "at least one of to or from or attach map types must be " + "specified, other map types are not permitted"); } auto updateVar = mapInfoOp.getVarPtr(); @@ -2046,7 +2049,19 @@ static LogicalResult verifyMapClause(Operation *op, OperandRange mapVars) { "present, mapper and iterator map type modifiers are permitted"); } - to ? updateToVars.insert(updateVar) : updateFromVars.insert(updateVar); + // It's possible we have an attach map, in which case if there is no to + // or from tied to it, we skip insertion. + if (to || from) { + to ? updateToVars.insert(updateVar) + : updateFromVars.insert(updateVar); + } + } + + if ((mapInfoOp.getVarPtrPtr() && !mapInfoOp.getVarPtrPtrType()) || + (!mapInfoOp.getVarPtrPtr() && mapInfoOp.getVarPtrPtrType())) { + return emitError( + op->getLoc(), + "both the varPtrPtr and varPtrPtrType must be present"); } } else if (!isa(op)) { return emitError(op->getLoc(), diff --git a/mlir/lib/Dialect/OpenMP/Transforms/OpenMPOffloadPrivatizationPrepare.cpp b/mlir/lib/Dialect/OpenMP/Transforms/OpenMPOffloadPrivatizationPrepare.cpp index c117d9b034b7a..d3182a51b3a3f 100644 --- a/mlir/lib/Dialect/OpenMP/Transforms/OpenMPOffloadPrivatizationPrepare.cpp +++ b/mlir/lib/Dialect/OpenMP/Transforms/OpenMPOffloadPrivatizationPrepare.cpp @@ -127,7 +127,7 @@ class PrepareForOMPOffloadPrivatizationPass // For boxchars this won't be a pointer. But, MapsForPrivatizedSymbols // should have mapped the pointer to the boxchar so use that as varPtr. Value varPtr = mapInfoOp.getVarPtr(); - Type varType = mapInfoOp.getVarType(); + Type varType = mapInfoOp.getVarPtrType(); bool isPrivatizedByValue = !isa(privVar.getType()); diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp index ed2321ad6426d..32b1fb70debe8 100644 --- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp +++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp @@ -3935,6 +3935,16 @@ static void collectMapDataFromMapOperands( llvm::IRBuilderBase &builder, ArrayRef useDevPtrOperands = {}, ArrayRef useDevAddrOperands = {}, ArrayRef hasDevAddrOperands = {}) { + + auto checkRefPtrOrPteeMapWithAttach = [](omp::ClauseMapFlags mapType) { + bool hasRefType = + checkHasClauseMapFlag(mapType, omp::ClauseMapFlags::ref_ptr) || + checkHasClauseMapFlag(mapType, omp::ClauseMapFlags::ref_ptee) || + checkHasClauseMapFlag(mapType, omp::ClauseMapFlags::ref_ptr_ptee); + return hasRefType && + checkHasClauseMapFlag(mapType, omp::ClauseMapFlags::attach); + }; + auto checkIsAMember = [](const auto &mapVars, auto mapOp) { // Check if this is a member mapping and correctly assign that it is, if // it is a member of a larger object. @@ -3955,10 +3965,7 @@ static void collectMapDataFromMapOperands( // Process MapOperands for (Value mapValue : mapVars) { auto mapOp = cast(mapValue.getDefiningOp()); - bool isRefPtrOrPteeMapWithAttach = - checkHasClauseMapFlag(mapOp.getMapType(), - omp::ClauseMapFlags::ref_ptr) && - checkHasClauseMapFlag(mapOp.getMapType(), omp::ClauseMapFlags::attach); + bool isRefPtrOrPteeMapWithAttach = checkRefPtrOrPteeMapWithAttach(mapOp.getMapType()); Value offloadPtr = (mapOp.getVarPtrPtr() && !isRefPtrOrPteeMapWithAttach) ? mapOp.getVarPtrPtr() : mapOp.getVarPtr(); mapData.OriginalValue.push_back(moduleTranslation.lookupValue(offloadPtr)); @@ -3979,11 +3986,27 @@ static void collectMapDataFromMapOperands( mapData.BasePointers.push_back(mapData.OriginalValue.back()); } - mapData.BaseType.push_back( - moduleTranslation.convertType(mapOp.getVarType())); - mapData.Sizes.push_back( - getSizeInBytes(dl, mapOp.getVarType(), mapOp, mapData.Pointers.back(), - mapData.BaseType.back(), builder, moduleTranslation)); + // In every situation we currently have if we have a varPtrPtr present + // we wish to utilise it's type for the base type, main cases are + // currently Fortran descriptor base address maps and attach maps. + mapData.BaseType.push_back(moduleTranslation.convertType( + mapOp.getVarPtrPtr() ? mapOp.getVarPtrPtrType().value() + : mapOp.getVarPtrType())); + + // For the attach map cases, it's a little odd, as we effectively have to + // utilise the base address (including all bounds offsets) for the pointer + // field, the pointer address for the base address field, and the pointer + // not the data (base addresses) size. So we end up with a mix of base + // types and sizes we wish to insert here. + mlir::Type sizeType = (isRefPtrOrPteeMapWithAttach || !mapOp.getVarPtrPtr()) + ? mapOp.getVarPtrType() + : mapOp.getVarPtrPtrType().value(); + + mapData.Sizes.push_back(getSizeInBytes( + dl, sizeType, isRefPtrOrPteeMapWithAttach ? nullptr : mapOp, + mapData.Pointers.back(), moduleTranslation.convertType(sizeType), + builder, moduleTranslation)); + mapData.MapClause.push_back(mapOp.getOperation()); mapData.Types.push_back(convertClauseMapFlags(mapOp.getMapType())); mapData.Names.push_back(LLVM::createMappingInformation( @@ -4038,7 +4061,7 @@ static void collectMapDataFromMapOperands( mapData.IsDeclareTarget.push_back(false); mapData.BasePointers.push_back(mapData.OriginalValue.back()); mapData.BaseType.push_back( - moduleTranslation.convertType(mapOp.getVarType())); + moduleTranslation.convertType(mapOp.getVarPtrType())); // If we're an attach map, we need to maintain the size currently, even // if we're not sending data, as the runtime (at least currently) @@ -4047,7 +4070,7 @@ static void collectMapDataFromMapOperands( llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ATTACH) == llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ATTACH) { mapData.Sizes.push_back(getSizeInBytes( - dl, mapOp.getVarType(), mapOp, mapData.Pointers.back(), + dl, mapOp.getVarPtrType(), mapOp, mapData.Pointers.back(), mapData.BaseType.back(), builder, moduleTranslation)); } else { mapData.Sizes.push_back(builder.getInt64(0)); @@ -4082,9 +4105,9 @@ static void collectMapDataFromMapOperands( mapData.Pointers.push_back(origValue); mapData.IsDeclareTarget.push_back(false); mapData.BaseType.push_back( - moduleTranslation.convertType(mapOp.getVarType())); + moduleTranslation.convertType(mapOp.getVarPtrType())); mapData.Sizes.push_back( - builder.getInt64(dl.getTypeSize(mapOp.getVarType()))); + builder.getInt64(dl.getTypeSize(mapOp.getVarPtrType()))); mapData.MapClause.push_back(mapOp.getOperation()); if (llvm::to_underlying(mapType & mapTypeAlways)) { // Descriptors are mapped with the ALWAYS flag, since they can get @@ -4390,7 +4413,15 @@ processIndividualMap(llvm::IRBuilderBase &builder, !isPtrTy) mapFlag |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_LITERAL; - if (memberOfFlag != llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE) + // if we have a pointer and it's part of a MEMBER_OF mapping we do not apply + // MEMBER_OF, as the runtime currently has a work-around that utilises + // MEMBER_OF to prevent reference updating in certain scenarios instead of + // target_param, however, this causes a noticable issue in cases where we + // map some data (Fortran descriptor primarily at the moment), alter it on + // the host, and then expect it to not be updated in a subsequent impliict map + // (such as an implicit map on a target). + if (memberOfFlag != llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE && + !isPtrTy) ompBuilder.setCorrectMemberOfFlag(mapFlag, memberOfFlag); // if we're provided a mapDataParentIdx, then the data being mapped is @@ -4703,7 +4734,29 @@ static void mapParentWithMembers( llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_CLOSE; ompBuilder.setCorrectMemberOfFlag(mapFlag, memberOfFlag); - if (targetDirective == TargetDirective::TargetUpdate || hasMapClose) { + // AG NOTE: Forcing it into the top path works fine, we could temporarily + // force any mappings with attach into this path till we work out a + // solution, which will leave some avenue for the optimization... we would + // have to make the attach a member of the derived type then, and it's not + // really designed to have more members at the current index. Another method + // might be to traverse the masp and look for other members with a shared + // descriptor/address and check... + // part of the issue here is likely that the base address (element 0) + // and the descriptors address, share the same initial address offset, + // so if we calcualte the size from them for the overlap, we end up with a size of + /// 0, which is fine normally but the NewlyAllocated check takes into account size... + // - What we can do: + // 1) adjust the lowering to take into account <= for the size part... unsure + // how accurate that would be long term + // 2) Adjust the size calculation here to use the size of the object if we have it pre-computed + // rather than self-calculate it / a conditional to select it. The issue with this, is that + // we likely haven't calculated the reality that it's a ptr and we're going to feed it 48, + // which technically isn't correrct... it should be 8... or nothing at all as it's either the + // data which isn't part of the object or the pointer in the descriptor, and it shouldn't really + // be both. + // AG NOTE: We will want to remove the target_param component of + // any processIndividualMap's that have ATTACH just to be safe. + // if (targetDirective == TargetDirective::TargetUpdate || hasMapClose) { combinedInfo.Types.emplace_back(mapFlag); combinedInfo.DevicePointers.emplace_back( mapData.DevicePointers[mapDataIndex]); @@ -4714,66 +4767,66 @@ static void mapParentWithMembers( mapData.BasePointers[mapDataIndex]); combinedInfo.Pointers.emplace_back(mapData.Pointers[mapDataIndex]); combinedInfo.Sizes.emplace_back(mapData.Sizes[mapDataIndex]); - } else { - llvm::SmallVector overlapIdxs; - // Find all of the members that "overlap", i.e. occlude other members that - // were mapped alongside the parent, e.g. member [0], occludes - getOverlappedMembers(overlapIdxs, mapData, parentClause); - // We need to make sure the overlapped members are sorted in order of - // lowest address to highest address - sortMapIndices(overlapIdxs, parentClause); - - lowAddr = builder.CreatePointerCast(mapData.Pointers[mapDataIndex], - builder.getPtrTy()); - highAddr = builder.CreatePointerCast( - builder.CreateConstGEP1_32(mapData.BaseType[mapDataIndex], - mapData.Pointers[mapDataIndex], 1), - builder.getPtrTy()); - - // TODO: We may want to skip arrays/array sections in this as Clang does - // so it appears to be an optimisation rather than a neccessity though, - // but this requires further investigation. However, we would have to make - // sure to not exclude maps with bounds that ARE pointers, as these are - // processed as seperate components, i.e. pointer + data. - for (auto v : overlapIdxs) { - auto mapDataOverlapIdx = getMapDataMemberIdx( - mapData, - cast(parentClause.getMembers()[v].getDefiningOp())); - combinedInfo.Types.emplace_back(mapFlag); - combinedInfo.DevicePointers.emplace_back( - mapData.DevicePointers[mapDataOverlapIdx]); - combinedInfo.Mappers.emplace_back(mapData.Mappers[mapDataOverlapIdx]); - combinedInfo.Names.emplace_back(LLVM::createMappingInformation( - mapData.MapClause[mapDataIndex]->getLoc(), ompBuilder)); - combinedInfo.BasePointers.emplace_back( - mapData.BasePointers[mapDataIndex]); - combinedInfo.Pointers.emplace_back(lowAddr); - combinedInfo.Sizes.emplace_back(builder.CreateIntCast( - builder.CreatePtrDiff(builder.getInt8Ty(), - mapData.OriginalValue[mapDataOverlapIdx], - lowAddr), - builder.getInt64Ty(), /*isSigned=*/true)); - lowAddr = builder.CreateConstGEP1_32( - checkIfPointerMap(llvm::cast( - mapData.MapClause[mapDataOverlapIdx])) - ? builder.getPtrTy() - : mapData.BaseType[mapDataOverlapIdx], - mapData.BasePointers[mapDataOverlapIdx], 1); - } - - combinedInfo.Types.emplace_back(mapFlag); - combinedInfo.DevicePointers.emplace_back( - mapData.DevicePointers[mapDataIndex]); - combinedInfo.Mappers.emplace_back(mapData.Mappers[mapDataIndex]); - combinedInfo.Names.emplace_back(LLVM::createMappingInformation( - mapData.MapClause[mapDataIndex]->getLoc(), ompBuilder)); - combinedInfo.BasePointers.emplace_back( - mapData.BasePointers[mapDataIndex]); - combinedInfo.Pointers.emplace_back(lowAddr); - combinedInfo.Sizes.emplace_back(builder.CreateIntCast( - builder.CreatePtrDiff(builder.getInt8Ty(), highAddr, lowAddr), - builder.getInt64Ty(), true)); - } + // } else { + // llvm::SmallVector overlapIdxs; + // // Find all of the members that "overlap", i.e. occlude other members that + // // were mapped alongside the parent, e.g. member [0], occludes + // getOverlappedMembers(overlapIdxs, mapData, parentClause); + // // We need to make sure the overlapped members are sorted in order of + // // lowest address to highest address + // sortMapIndices(overlapIdxs, parentClause); + + // lowAddr = builder.CreatePointerCast(mapData.Pointers[mapDataIndex], + // builder.getPtrTy()); + // highAddr = builder.CreatePointerCast( + // builder.CreateConstGEP1_32(mapData.BaseType[mapDataIndex], + // mapData.Pointers[mapDataIndex], 1), + // builder.getPtrTy()); + + // // TODO: We may want to skip arrays/array sections in this as Clang does + // // so it appears to be an optimisation rather than a neccessity though, + // // but this requires further investigation. However, we would have to make + // // sure to not exclude maps with bounds that ARE pointers, as these are + // // processed as seperate components, i.e. pointer + data. + // for (auto v : overlapIdxs) { + // auto mapDataOverlapIdx = getMapDataMemberIdx( + // mapData, + // cast(parentClause.getMembers()[v].getDefiningOp())); + // combinedInfo.Types.emplace_back(mapFlag); + // combinedInfo.DevicePointers.emplace_back( + // mapData.DevicePointers[mapDataOverlapIdx]); + // combinedInfo.Mappers.emplace_back(mapData.Mappers[mapDataOverlapIdx]); + // combinedInfo.Names.emplace_back(LLVM::createMappingInformation( + // mapData.MapClause[mapDataIndex]->getLoc(), ompBuilder)); + // combinedInfo.BasePointers.emplace_back( + // mapData.BasePointers[mapDataIndex]); + // combinedInfo.Pointers.emplace_back(lowAddr); + // combinedInfo.Sizes.emplace_back(builder.CreateIntCast( + // builder.CreatePtrDiff(builder.getInt8Ty(), + // mapData.OriginalValue[mapDataOverlapIdx], + // lowAddr), + // builder.getInt64Ty(), /*isSigned=*/true)); + // lowAddr = builder.CreateConstGEP1_32( + // checkIfPointerMap(llvm::cast( + // mapData.MapClause[mapDataOverlapIdx])) + // ? builder.getPtrTy() + // : mapData.BaseType[mapDataOverlapIdx], + // mapData.BasePointers[mapDataOverlapIdx], 1); + // } + + // combinedInfo.Types.emplace_back(mapFlag); + // combinedInfo.DevicePointers.emplace_back( + // mapData.DevicePointers[mapDataIndex]); + // combinedInfo.Mappers.emplace_back(mapData.Mappers[mapDataIndex]); + // combinedInfo.Names.emplace_back(LLVM::createMappingInformation( + // mapData.MapClause[mapDataIndex]->getLoc(), ompBuilder)); + // combinedInfo.BasePointers.emplace_back( + // mapData.BasePointers[mapDataIndex]); + // combinedInfo.Pointers.emplace_back(lowAddr); + // combinedInfo.Sizes.emplace_back(builder.CreateIntCast( + // builder.CreatePtrDiff(builder.getInt8Ty(), highAddr, lowAddr), + // builder.getInt64Ty(), true)); + // } } } @@ -4884,9 +4937,16 @@ createAlteredByCaptureMap(MapInfoData &mapData, assert(!moduleTranslation.getOpenMPBuilder()->Config.isTargetDevice() && "function only supported for host device codegen"); for (size_t i = 0; i < mapData.MapClause.size(); ++i) { - // if it's declare target, skip it, it's handled separately. - if (!mapData.IsDeclareTarget[i]) { - auto mapOp = cast(mapData.MapClause[i]); + auto mapOp = cast(mapData.MapClause[i]); + bool isAttachMap = + ((convertClauseMapFlags(mapOp.getMapType()) & + llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ATTACH) == + llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ATTACH); + + // if it's declare target, skip it, it's handled separately. However, if + // it's declare target, and an attach map, we want to calculate the exact + // address offset so that we attach correctly. + if (!mapData.IsDeclareTarget[i] || (mapData.IsDeclareTarget[i] && isAttachMap)) { omp::VariableCaptureKind captureKind = mapOp.getMapCaptureType(); bool isPtrTy = checkIfPointerMap(mapOp); @@ -5653,7 +5713,7 @@ createDeviceArgumentAccessor(MapInfoData &mapData, llvm::Argument &arg, capture = mapOp.getMapCaptureType(); // Get information of alignment of mapped object alignmentValue = typeToLLVMIRTranslator.getPreferredAlignment( - mapOp.getVarType(), ompBuilder.M.getDataLayout()); + mapOp.getVarPtrType(), ompBuilder.M.getDataLayout()); break; } @@ -6125,7 +6185,7 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder, // So, we don't store it in any datastructure. Instead, we just // do some sanity checks on it right now. auto mapInfoOp = mappedValue.getDefiningOp(); - [[maybe_unused]] Type varType = mapInfoOp.getVarType(); + [[maybe_unused]] Type varType = mapInfoOp.getVarPtrType(); // Check #1: Check that the type of the private variable matches // the type of the variable being mapped.