From eca9112402f207bf9a59affa383b5258e4e846a1 Mon Sep 17 00:00:00 2001 From: agozillon Date: Fri, 20 Sep 2024 22:57:18 -0500 Subject: [PATCH] Implement N-D bounds and tidy up, still small bug with N-D parent subscripts to work through --- flang/lib/Lower/DirectivesCommon.h | 5 + flang/lib/Lower/OpenMP/ClauseProcessor.cpp | 9 +- flang/lib/Lower/OpenMP/Utils.cpp | 174 +++++------------- .../OpenMP/OpenMPToLLVMIRTranslation.cpp | 4 + mlir/lib/Target/LLVMIR/ModuleTranslation.cpp | 2 + 5 files changed, 66 insertions(+), 128 deletions(-) diff --git a/flang/lib/Lower/DirectivesCommon.h b/flang/lib/Lower/DirectivesCommon.h index d2060e77ce5305..14e6059fd91812 100644 --- a/flang/lib/Lower/DirectivesCommon.h +++ b/flang/lib/Lower/DirectivesCommon.h @@ -1023,6 +1023,7 @@ genBoundsOps(fir::FirOpBuilder &builder, mlir::Location loc, } if (!triplet) { + llvm::errs() << "not a triplet, extent set to 1 \n"; // If it is a scalar subscript, then the upper bound // is equal to the lower bound, and the extent is one. ubound = lbound; @@ -1067,6 +1068,7 @@ genBoundsOps(fir::FirOpBuilder &builder, mlir::Location loc, if (info.isPresent && mlir::isa( fir::unwrapRefType(info.addr.getType()))) { + llvm::errs() << "extent from box? \n"; extent = builder .genIfOp(loc, idxTy, info.isPresent, /*withElseRegion=*/true) @@ -1140,6 +1142,7 @@ AddrAndBoundsInfo gatherDataOperandAddrAndBounds( AddrAndBoundsInfo info; + llvm::errs() << "treatIndexAsSection: " << treatIndexAsSection << "\n"; if (!maybeDesignator) { info = getDataOperandBaseAddr(converter, builder, symbol, operandLocation); asFortran << symbol->name().ToString(); @@ -1150,6 +1153,7 @@ AddrAndBoundsInfo gatherDataOperandAddrAndBounds( if ((designator.Rank() > 0 || treatIndexAsSection) && IsArrayElement(designator)) { + llvm::errs() << "treating index as section? \n"; auto arrayRef = detail::getRef(designator); // This shouldn't fail after IsArrayElement(designator). assert(arrayRef && "Expecting ArrayRef"); @@ -1191,6 +1195,7 @@ AddrAndBoundsInfo gatherDataOperandAddrAndBounds( } if (!arrayRef->subscript().empty()) { + llvm::errs() << "generating bounds from subscript? \n"; asFortran << '('; bounds = genBoundsOps( builder, operandLocation, converter, stmtCtx, arrayRef->subscript(), diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp index 642fb2634fc08b..a1e4ff9cd5d0a6 100644 --- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp +++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp @@ -914,9 +914,12 @@ void ClauseProcessor::processMapObjects( assert(!objectList.empty() && "could not find parent objects of derived type member"); parentObj = objectList[0]; - auto insert = parentMemberIndices.emplace(parentObj.value(), - OmpMapParentAndMemberData{}); - insert.first->second.parentObjList.push_back(parentObj.value()); + auto found = parentMemberIndices.find(parentObj.value()); + if (found != parentMemberIndices.end()) + found->second.parentObjList.push_back(parentObj.value()); + else + parentMemberIndices.emplace(parentObj.value(), + OmpMapParentAndMemberData{}); if (isMemberOrParentAllocatableOrPointer(object, semaCtx)) { llvm::SmallVector indices; diff --git a/flang/lib/Lower/OpenMP/Utils.cpp b/flang/lib/Lower/OpenMP/Utils.cpp index 711cc8b31368e1..7291d5b7920790 100644 --- a/flang/lib/Lower/OpenMP/Utils.cpp +++ b/flang/lib/Lower/OpenMP/Utils.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include @@ -171,12 +172,12 @@ bool isDuplicateMemberMapInfo(OmpMapParentAndMemberData &parentMembers, return false; } -void generateArrayIndices(lower::AbstractConverter &converter, +static void generateArrayIndices(lower::AbstractConverter &converter, + fir::FirOpBuilder &firOpBuilder, lower::StatementContext &stmtCtx, mlir::Location clauseLocation, llvm::SmallVectorImpl &indices, omp::Object object) { - fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder(); if (auto maybeRef = evaluate::ExtractDataRef(*object.ref())) { evaluate::DataRef ref = *maybeRef; if (auto *arr = std::get_if(&ref.u)) { @@ -200,12 +201,11 @@ void generateArrayIndices(lower::AbstractConverter &converter, } } -mlir::Value generateBoundsComparisonBranch(lower::AbstractConverter &converter, +static mlir::Value generateBoundsComparisonBranch(fir::FirOpBuilder &firOpBuilder, mlir::Location clauseLocation, mlir::arith::CmpIPredicate pred, - mlir::Value &index, + mlir::Value index, mlir::Value bound) { - fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder(); auto cmp = firOpBuilder.create(clauseLocation, pred, index, bound); return firOpBuilder @@ -220,6 +220,34 @@ mlir::Value generateBoundsComparisonBranch(lower::AbstractConverter &converter, .getResults()[0]; } +static void extendBoundsFromMultipleSubscripts( + lower::AbstractConverter &converter, lower::StatementContext &stmtCtx, + mlir::omp::MapInfoOp mapOp, omp::ObjectList objList) { + fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder(); + if (!mapOp.getBounds().empty()) { + for (omp::Object v : objList) { + llvm::SmallVector indices; + generateArrayIndices(converter, firOpBuilder, stmtCtx, mapOp.getLoc(), + indices, v); + + for (size_t i = 0; i < mapOp.getBounds().size(); ++i) { + if (auto boundOp = mlir::dyn_cast_if_present( + mapOp.getBounds()[i].getDefiningOp())) { + boundOp.getUpperBoundMutable().assign(generateBoundsComparisonBranch( + firOpBuilder, mapOp.getLoc(), mlir::arith::CmpIPredicate::ugt, + indices[i], boundOp.getUpperBoundMutable().begin()->get())); + boundOp.getLowerBoundMutable().assign(generateBoundsComparisonBranch( + firOpBuilder, mapOp.getLoc(), mlir::arith::CmpIPredicate::ult, + indices[i], boundOp.getLowerBoundMutable().begin()->get())); + } + } + } + } + + // reorder all SSA's we may have generated to make sure we maintain ordering. + sortTopologically(mapOp->getBlock()); +} + // When mapping members of derived types, there is a chance that one of the // members along the way to a mapped member is an descriptor. In which case // we have to make sure we generate a map for those along the way otherwise @@ -261,8 +289,8 @@ mlir::Value createParentSymAndGenIntermediateMaps( fir::unwrapPassByRefType(curValue.getType()))) { if (arrayExprWithSubscript(objectList[i])) { llvm::SmallVector indices; - generateArrayIndices(converter, stmtCtx, clauseLocation, indices, - objectList[i]); + generateArrayIndices(converter, firOpBuilder, stmtCtx, clauseLocation, + indices, objectList[i]); assert(!indices.empty() && "missing expected indices for map clause"); curValue = firOpBuilder.create( clauseLocation, firOpBuilder.getRefType(arrType.getEleTy()), @@ -332,37 +360,9 @@ mlir::Value createParentSymAndGenIntermediateMaps( interimIndices); auto v = std::distance( parentMemberIndices.memberPlacementIndices.begin(), it); - - auto savedIP = firOpBuilder.getInsertionPoint(); - firOpBuilder.setInsertionPoint( - parentMemberIndices.memberMap[v].getBounds()[0].getDefiningOp()); - - llvm::SmallVector indices; - generateArrayIndices(converter, stmtCtx, clauseLocation, indices, - objectList[i]); - - for (size_t i = 0; - i < parentMemberIndices.memberMap[v].getBounds().size(); i++) { - if (auto boundOp = mlir::dyn_cast_if_present( - parentMemberIndices.memberMap[v] - .getBounds()[i] - .getDefiningOp())) { - boundOp.getUpperBoundMutable().assign( - generateBoundsComparisonBranch( - converter, clauseLocation, mlir::arith::CmpIPredicate::ugt, - indices[i], boundOp.getUpperBoundMutable().begin()->get())); - boundOp.getLowerBoundMutable().assign( - generateBoundsComparisonBranch( - converter, clauseLocation, mlir::arith::CmpIPredicate::ult, - indices[i], boundOp.getLowerBoundMutable().begin()->get())); - } - } - - firOpBuilder.setInsertionPoint(parentMemberIndices.memberMap[v] - .getBounds()[0] - .getDefiningOp() - ->getBlock(), - savedIP); + extendBoundsFromMultipleSubscripts(converter, stmtCtx, + parentMemberIndices.memberMap[v], + {objectList[i]}); } curValue = firOpBuilder.create(clauseLocation, curValue); @@ -473,14 +473,6 @@ void insertChildMapInfoIntoParent( if (parentExists) { auto mapOp = llvm::cast( mapOperands[parentIdx].getDefiningOp()); - // NOTE: To maintain appropriate SSA ordering, we move the parent map - // which will now have references to its children after the last - // of its members to be generated. This is necessary when a user - // has defined a series of parent and children maps where the parent - // precedes the children. An alternative, may be to do - // delayed generation of map info operations from the clauses and - // organize them first before generation. - mapOp->moveAfter(indices.second.memberMap.back()); for (mlir::omp::MapInfoOp memberMap : indices.second.memberMap) mapOp.getMembersMutable().append(memberMap.getResult()); @@ -488,35 +480,15 @@ void insertChildMapInfoIntoParent( mapOp.setMembersIndexAttr(firOpBuilder.create2DIntegerArrayAttr( indices.second.memberPlacementIndices)); - if (!mapOp.getBounds().empty()) { - auto savedIP = firOpBuilder.getInsertionPoint(); - firOpBuilder.setInsertionPoint(mapOp.getBounds()[0].getDefiningOp()); - - for (auto v : indices.second.parentObjList) { - llvm::SmallVector indexes; - generateArrayIndices(converter, stmtCtx, mapOp.getLoc(), indexes, v); - - for (size_t i = 0; i < mapOp.getBounds().size(); i++) { - if (auto boundOp = - mlir::dyn_cast_if_present( - mapOp.getBounds()[i].getDefiningOp())) { - boundOp.getUpperBoundMutable().assign( - generateBoundsComparisonBranch( - converter, mapOp.getLoc(), - mlir::arith::CmpIPredicate::ugt, indexes[i], - boundOp.getUpperBoundMutable().begin()->get())); - boundOp.getLowerBoundMutable().assign( - generateBoundsComparisonBranch( - converter, mapOp.getLoc(), - mlir::arith::CmpIPredicate::ult, indexes[i], - boundOp.getLowerBoundMutable().begin()->get())); - } - } - } - - firOpBuilder.setInsertionPoint( - mapOp.getBounds()[0].getDefiningOp()->getBlock(), savedIP); - } + // Not only does this extend bounds if multiple subscripts are + // defined for a map parent, but it also performs a topological + // sort, re-ordering SSA values so everything maintains correct + // ordering, this by extension shuffles the parent map, into the + // correct position after it's member definitions, as when we fall + // into this segment of the if statement, the parent map information + // has been generated prior to it's members in most cases. + extendBoundsFromMultipleSubscripts(converter, stmtCtx, mapOp, + indices.second.parentObjList); } 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 @@ -525,7 +497,6 @@ void insertChildMapInfoIntoParent( // the "main" map type clause from the directive being used. uint64_t mapType = indices.second.memberMap[0].getMapType().value_or(0); - llvm::errs() << "Not parent exists \n"; llvm::SmallVector members; for (mlir::omp::MapInfoOp memberMap : indices.second.memberMap) members.push_back(memberMap.getResult()); @@ -550,55 +521,8 @@ void insertChildMapInfoIntoParent( info.rawInput.getType(), /*partialMap=*/true); - // TODO: - // > Fix all check-all tests that are broken - // > Fix parent mapping, - // probably needs to be elsewhere rather than below, i.e. whenever we - // find parent sym more than once which may be in the regular - // proccessMap - // as i think above is just checking if we have alreadty generated it to - // insert. so it's likely in the processmapObject where we first insert - // the parent symbol - // + map info, and in these cases we'll have to adjust the member indices - // > check N-D indexing works, it likely will need some minor - // adjustments, we can hold off on it until everything - // else is done as an extra addition - - // for (auto v : indices.second.parentObjList) { - // v.ref()->dump(); - // v.sym()->dump(); - // } - - if (!mapOp.getBounds().empty()) { - auto savedIP = firOpBuilder.getInsertionPoint(); - firOpBuilder.setInsertionPoint(mapOp.getBounds()[0].getDefiningOp()); - - for (auto v : indices.second.parentObjList) { - llvm::SmallVector indexes; - generateArrayIndices(converter, stmtCtx, mapOp.getLoc(), indexes, v); - - for (size_t i = 0; i < mapOp.getBounds().size(); i++) { - if (auto boundOp = - mlir::dyn_cast_if_present( - mapOp.getBounds()[i].getDefiningOp())) { - boundOp.getUpperBoundMutable().assign( - generateBoundsComparisonBranch( - converter, mapOp.getLoc(), - mlir::arith::CmpIPredicate::ugt, indexes[i], - boundOp.getUpperBoundMutable().begin()->get())); - boundOp.getLowerBoundMutable().assign( - generateBoundsComparisonBranch( - converter, mapOp.getLoc(), - mlir::arith::CmpIPredicate::ult, indexes[i], - boundOp.getLowerBoundMutable().begin()->get())); - } - } - } - - firOpBuilder.setInsertionPoint( - mapOp.getBounds()[0].getDefiningOp()->getBlock(), savedIP); - } - + extendBoundsFromMultipleSubscripts(converter, stmtCtx, mapOp, + indices.second.parentObjList); mapOperands.push_back(mapOp); if (mapSymTypes) mapSymTypes->push_back(mapOp.getType()); diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp index 8a1911bb50ebcd..0aed0f35740f83 100644 --- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp +++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp @@ -2703,6 +2703,7 @@ calculateBoundsOffset(LLVM::ModuleTranslation &moduleTranslation, // into the array. We currently do reverse order of the bounds, which // I believe leans more towards Fortran's column-major in memory. if (isArrayTy) { + llvm::errs() << "array type... ? \n"; idx.push_back(builder.getInt64(0)); for (int i = bounds.size() - 1; i >= 0; --i) { if (auto boundOp = dyn_cast_if_present( @@ -2711,6 +2712,7 @@ calculateBoundsOffset(LLVM::ModuleTranslation &moduleTranslation, } } } else { + llvm::errs() << "not array type! \n"; // If we do not have an array type, but we have bounds, then we're dealing // with a pointer that's being treated like an array and we have the // underlying type e.g. an i32, or f64 etc, e.g. a fortran descriptor base @@ -2738,6 +2740,8 @@ calculateBoundsOffset(LLVM::ModuleTranslation &moduleTranslation, dimensionIndexSizeOffset[i - 1])); } } + + // extent = 1, probably why there's an issue... // Now that we have calculated how much we move by per index, we must // multiply each lower bound offset in indexes by the size offset we diff --git a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp index fcb329eb7a92c1..94d9ad2a89c856 100644 --- a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp +++ b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp @@ -2000,6 +2000,8 @@ mlir::translateModuleToLLVMIR(Operation *module, llvm::LLVMContext &llvmContext, if (failed(translator.convertFunctions())) return nullptr; + module->dump(); + // Once we've finished constructing elements in the module, we should convert // it to use the debug info format desired by LLVM. // See https://llvm.org/docs/RemoveDIsDebugInfo.html