llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-flang-fir-hlfir Author: None (agozillon) <details> <summary>Changes</summary> This PR is one of four required to implement the attach mapping semantics in Flang, alongside the ref_ptr/ref_ptee/ref_ptr_ptee map modifiers and the attach(always/never/auto) modifiers. This PR is the MapInfoFinalization changes required to support these features, it mainly deals with applying the correct attach map type and manipulating the descriptor types maps for base address and descriptor so that when we specify ref_ptr/ref_ptee we emit one of the two maps and when we emit ref_ptr_ptee we emit our usual default maps. In all cases we add the "glue" of an new attach map except in cases where a user has provided attach never. In cases where we are provided an always, we apply the always map type to our attach maps. It's important to note the runtime has a toggle for the auto map behavior, which will flip the attach behavior to the newer semantics or the older semantics for backwards compatibility (outside the purview of this PR but good to mention). --- Patch is 188.37 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/177715.diff 19 Files Affected: - (modified) flang/lib/Lower/OpenMP/Utils.cpp (+17-7) - (modified) flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp (+508-248) - (modified) flang/test/Lower/OpenMP/DelayedPrivatization/target-private-allocatable.f90 (+3-2) - (modified) flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90 (+1-1) - (modified) flang/test/Lower/OpenMP/allocatable-map.f90 (+3-1) - (modified) flang/test/Lower/OpenMP/array-bounds.f90 (+5-3) - (modified) flang/test/Lower/OpenMP/attach-and-ref-modifier.f90 (+55-7) - (modified) flang/test/Lower/OpenMP/declare-mapper.f90 (+6-4) - (modified) flang/test/Lower/OpenMP/derived-type-allocatable-map.f90 (+18-12) - (modified) flang/test/Lower/OpenMP/derived-type-map.f90 (+10-9) - (modified) flang/test/Lower/OpenMP/map-character.f90 (+8-2) - (modified) flang/test/Lower/OpenMP/map-component-ref.f90 (+1-1) - (modified) flang/test/Lower/OpenMP/map-descriptor-deferral.f90 (+14-7) - (modified) flang/test/Lower/OpenMP/optional-argument-map-2.f90 (+21-19) - (modified) flang/test/Lower/OpenMP/target.f90 (+3-1) - (modified) flang/test/Lower/OpenMP/use-device-ptr-to-use-device-addr.f90 (+5-5) - (modified) flang/test/Lower/volatile-openmp.f90 (+5-3) - (modified) flang/test/Transforms/omp-map-info-finalization-implicit-field.fir (+9-2) - (modified) flang/test/Transforms/omp-map-info-finalization.fir (+34-21) ``````````diff diff --git a/flang/lib/Lower/OpenMP/Utils.cpp b/flang/lib/Lower/OpenMP/Utils.cpp index dce8580856664..fbb41d489ede2 100644 --- a/flang/lib/Lower/OpenMP/Utils.cpp +++ b/flang/lib/Lower/OpenMP/Utils.cpp @@ -562,6 +562,13 @@ mlir::Value createParentSymAndGenIntermediateMaps( interimMapType &= ~mlir::omp::ClauseMapFlags::to; interimMapType &= ~mlir::omp::ClauseMapFlags::from; interimMapType &= ~mlir::omp::ClauseMapFlags::return_param; + // We do not want to carry over the separation of descriptor and pointer + // mapping of any intermediate components we emit maps for as this can + // result in very odd differing behaviour when either ref_ptr/ptee is + // specified. + interimMapType &= ~mlir::omp::ClauseMapFlags::ref_ptr; + interimMapType &= ~mlir::omp::ClauseMapFlags::ref_ptee; + interimMapType &= ~mlir::omp::ClauseMapFlags::ref_ptr_ptee; // Create a map for the intermediate member and insert it and it's // indices into the parentMemberIndices list to track it. @@ -704,13 +711,16 @@ void insertChildMapInfoIntoParent( mapOp.setMembersIndexAttr(firOpBuilder.create2DI64ArrayAttr( indices.second.memberPlacementIndices)); } else { - // NOTE: We take the map type of the first child, this may not - // be the correct thing to do, however, we shall see. For the moment - // it allows this to work with enter and exit without causing MLIR - // verification issues. The more appropriate thing may be to take - // the "main" map type clause from the directive being used. - mlir::omp::ClauseMapFlags mapType = - indices.second.memberMap[0].getMapType(); + // NOTE: We do not assign default mapped parents a map type, as + // selecting a childs can result in the incorrect map type being + // applied to the parent and data being incorrectly moved to or + // from device. We make an exception currently for present. + mlir::omp::ClauseMapFlags mapType = mlir::omp::ClauseMapFlags::storage; + + for (mlir::omp::MapInfoOp memberMap : indices.second.memberMap) + if ((memberMap.getMapType() & mlir::omp::ClauseMapFlags::present) == + mlir::omp::ClauseMapFlags::present) + mapType |= mlir::omp::ClauseMapFlags::present; llvm::SmallVector<mlir::Value> members; members.reserve(indices.second.memberMap.size()); diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp index 48f6009890b56..ec327c1312b1d 100644 --- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp +++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp @@ -60,6 +60,7 @@ namespace { class MapInfoFinalizationPass : public flangomp::impl::MapInfoFinalizationPassBase< MapInfoFinalizationPass> { + /// Helper class tracking a members parent and its /// placement in the parents member list struct ParentAndPlacement { @@ -77,9 +78,15 @@ class MapInfoFinalizationPass /// | | std::map<mlir::Operation *, mlir::Value> localBoxAllocas; - // List of deferrable descriptors to process at the end of - // the pass. - llvm::SmallVector<mlir::Operation *> deferrableDesc; + /// List of deferrable descriptors to process at the end of + /// the pass and their associated attach map if it exists. + llvm::SmallVector<std::pair<mlir::Operation *, mlir::Operation *>> + deferrableDesc; + + /// List of base addresses already expanded from their + /// descriptors within a parent, currently used to + /// prevent incorrect member index generation. + std::map<mlir::Operation *, llvm::SmallVector<uint64_t>> expandedBaseAddr; /// Return true if the given path exists in a list of paths. static bool @@ -150,6 +157,7 @@ class MapInfoFinalizationPass // Local de-dup within this op invocation. if (containsPath(newMemberIndexPaths, indexPath)) return; + // Global de-dup against already present member indices. if (mappedIndexPathExists(op, indexPath)) return; @@ -344,13 +352,26 @@ class MapInfoFinalizationPass return alloca; } + mlir::omp::ClauseMapFlags + removeAttachModifiers(mlir::omp::ClauseMapFlags mapType) { + // We can remove these maps as the lowering to LLVM-IR and the runtime have + // no requirement for these, it's primarily an indicator for this and + // similar passes. This is of course subject to change if we find need + // for it. + mapType &= ~mlir::omp::ClauseMapFlags::attach_always; + mapType &= ~mlir::omp::ClauseMapFlags::attach_never; + mapType &= ~mlir::omp::ClauseMapFlags::attach_auto; + return mapType; + } + /// Function that generates a FIR operation accessing the descriptor's /// base address (BoxOffsetOp) and a MapInfoOp for it. The most /// important thing to note is that we normally move the bounds from /// the descriptor map onto the base address map. mlir::omp::MapInfoOp - genBaseAddrMap(mlir::Value descriptor, mlir::OperandRange bounds, + genBaseAddrMap(mlir::Value descriptor, mlir::omp::MapInfoOp parentOp, mlir::omp::ClauseMapFlags mapType, fir::FirOpBuilder &builder, + bool isRefPtee = false, mlir::FlatSymbolRefAttr mapperId = mlir::FlatSymbolRefAttr()) { mlir::Location loc = descriptor.getLoc(); mlir::Value baseAddrAddr = fir::BoxOffsetOp::create( @@ -371,12 +392,14 @@ class MapInfoFinalizationPass return mlir::omp::MapInfoOp::create( builder, loc, baseAddrAddr.getType(), descriptor, mlir::TypeAttr::get(underlyingDescType), - builder.getAttr<mlir::omp::ClauseMapFlagsAttr>(mapType), + builder.getAttr<mlir::omp::ClauseMapFlagsAttr>( + removeAttachModifiers(mapType)), builder.getAttr<mlir::omp::VariableCaptureKindAttr>( mlir::omp::VariableCaptureKind::ByRef), baseAddrAddr, mlir::TypeAttr::get(underlyingBaseAddrType), - /*members=*/mlir::SmallVector<mlir::Value>{}, - /*membersIndex=*/mlir::ArrayAttr{}, bounds, + isRefPtee ? parentOp.getMembers() : mlir::SmallVector<mlir::Value>{}, + isRefPtee ? parentOp.getMembersIndexAttr() : mlir::ArrayAttr{}, + parentOp.getBounds(), /*mapperId=*/mapperId, /*name=*/builder.getStringAttr(""), /*partial_map=*/builder.getBoolAttr(false)); @@ -391,29 +414,237 @@ class MapInfoFinalizationPass /// of the base address index. void adjustMemberIndices( llvm::SmallVectorImpl<llvm::SmallVector<int64_t>> &memberIndices, - size_t memberIndex) { - llvm::SmallVector<int64_t> baseAddrIndex = memberIndices[memberIndex]; + ParentAndPlacement parentAndPlacement) { + llvm::SmallVector<int64_t> baseAddrIndex = + memberIndices[parentAndPlacement.index]; + auto &expansionIndexes = expandedBaseAddr[parentAndPlacement.parent]; // If we find another member that is "derived/a member of" the descriptor // that is not the descriptor itself, we must insert a 0 for the new base // address we have just added for the descriptor into the list at the // appropriate position to maintain correctness of the positional/index data // for that member. - for (llvm::SmallVector<int64_t> &member : memberIndices) + for (auto [i, member] : llvm::enumerate(memberIndices)) { + if (std::find(expansionIndexes.begin(), expansionIndexes.end(), i) != + expansionIndexes.end()) + if (member.size() == baseAddrIndex.size() + 1 && + member[baseAddrIndex.size()] == 0) + continue; + if (member.size() > baseAddrIndex.size() && std::equal(baseAddrIndex.begin(), baseAddrIndex.end(), member.begin())) member.insert(std::next(member.begin(), baseAddrIndex.size()), 0); + } // Add the base address index to the main base address member data baseAddrIndex.push_back(0); - // Insert our newly created baseAddrIndex into the larger list of indices at - // the correct location. - memberIndices.insert(std::next(memberIndices.begin(), memberIndex + 1), + uint64_t newIdxInsert = parentAndPlacement.index + 1; + expansionIndexes.push_back(newIdxInsert); + + // Insert our newly created baseAddrIndex into the larger list of + // indices at the correct location. + memberIndices.insert(std::next(memberIndices.begin(), newIdxInsert), baseAddrIndex); } + void + insertIntoMapClauseInterface(mlir::Operation *target, + std::function<void(mlir::MutableOperandRange &, + mlir::Operation *, unsigned)> + addOperands) { + auto argIface = + llvm::dyn_cast<mlir::omp::BlockArgOpenMPOpInterface>(target); + + if (auto mapClauseOwner = + llvm::dyn_cast<mlir::omp::MapClauseOwningOpInterface>(target)) { + mlir::MutableOperandRange mapVarsArr = mapClauseOwner.getMapVarsMutable(); + unsigned blockArgInsertIndex = + argIface + ? argIface.getMapBlockArgsStart() + argIface.numMapBlockArgs() + : 0; + addOperands(mapVarsArr, + llvm::dyn_cast_if_present<mlir::omp::TargetOp>( + argIface.getOperation()), + blockArgInsertIndex); + } + + if (auto targetDataOp = llvm::dyn_cast<mlir::omp::TargetDataOp>(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<mlir::omp::TargetOp>(target)) { + mlir::MutableOperandRange hasDevAddrMutableOpRange = + targetOp.getHasDeviceAddrVarsMutable(); + addOperands(hasDevAddrMutableOpRange, target, + argIface.getHasDeviceAddrBlockArgsStart() + + argIface.numHasDeviceAddrBlockArgs()); + } + } + + // 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<ParentAndPlacement> &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<mlir::Value> 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); + }; + + insertIntoMapClauseInterface(target, addOperands); + } + + // 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<mlir::omp::MapClauseOwningOpInterface>( + 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<mlir::Value> newMapOps; + newMapOps.reserve(mapVarsArr.size()); + llvm::for_each(mapVarsArr.getAsOperandRange(), + [&newMapOps](mlir::Value oper) { + if (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); + }; + + insertIntoMapClauseInterface(target, addOperands); + } + + // 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<mlir::omp::TargetOp, mlir::omp::TargetDataOp, + mlir::omp::TargetUpdateOp, mlir::omp::TargetExitDataOp, + mlir::omp::TargetEnterDataOp, + mlir::omp::DeclareMapperInfoOp>(user)) + return user; + + if (auto mapUser = llvm::dyn_cast<mlir::omp::MapInfoOp>(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 @@ -432,16 +663,23 @@ class MapInfoFinalizationPass /// allowing `to` mappings, and `target update` not allowing both `to` and /// `from` simultaneously. We currently try to maintain the `implicit` flag /// where necessary, although it does not seem strictly required. + /// + /// Currently, if it is a has_device_addr clause, we opt to not apply the + /// descriptor tag to it as it's used differently to a regular mapping + /// 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<mlir::omp::TargetExitDataOp, - mlir::omp::TargetUpdateOp>(target)) + mlir::omp::TargetUpdateOp>(target)) { return mapTypeFlag; + } - mapFlags flags = - mapFlags::to | (mapTypeFlag & (mapFlags::implicit | mapFlags::always)); + flags |= mapFlags::to | (mapTypeFlag & mapFlags::implicit); // Descriptors for objects will always be copied. This is because the // descriptor can be rematerialized by the compiler, and so the address @@ -496,6 +734,44 @@ class MapInfoFinalizationPass return false; } + [[maybe_unused]] mlir::Operation *genImplicitAttachMap( + mlir::omp::MapInfoOp descMapOp, mlir::Value descriptor, + llvm::SmallVectorImpl<ParentAndPlacement> &mapMemberUsers, + mlir::Operation *target, fir::FirOpBuilder &builder, + mlir::omp::ClauseMapFlags refFlagType, bool isAttachAlways = false) { + auto baseAddrAddr = fir::BoxOffsetOp::create( + builder, descMapOp->getLoc(), descriptor, fir::BoxFieldAttr::base_addr); + mlir::Type underlyingVarType = + llvm::cast<mlir::omp::PointerLikeType>( + fir::unwrapRefType(baseAddrAddr.getType())) + .getElementType(); + if (auto seqType = llvm::dyn_cast<fir::SequenceType>(underlyingVarType)) + if (seqType.hasDynamicExtents()) + underlyingVarType = seqType.getEleTy(); + + auto implicitAttachMap = mlir::omp::MapInfoOp::create( + builder, descMapOp->getLoc(), descMapOp.getResult().getType(), + descriptor, + mlir::TypeAttr::get(fir::unwrapRefType(descriptor.getType())), + builder.getAttr<mlir::omp::ClauseMapFlagsAttr>( + mlir::omp::ClauseMapFlags::attach | refFlagType | + (isAttachAlways ? mlir::omp::ClauseMapFlags::always + : mlir::omp::ClauseMapFlags::none)), + descMapOp.getMapCaptureTypeAttr(), /*varPtrPtr=*/ + baseAddrAddr, mlir::TypeAttr::get(underlyingVarType), + /*members=*/mlir::SmallVector<mlir::Value>{}, + /*membersInde... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/177715 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
