Implement N-D bounds and tidy up, still small bug with N-D parent sub…

…scripts to work through

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::BaseBoxType>(
  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<evaluate::ArrayRef>(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<BoundsOp, BoundsType>(
  builder, operandLocation, converter, stmtCtx, arrayRef->subscript(),

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<int64_t> indices;

flang/lib/Lower/OpenMP/Utils.cpp

@@ -27,6 +27,7 @@
 #include <flang/Parser/tools.h>
 #include <flang/Semantics/tools.h>
 #include <llvm/Support/CommandLine.h>
+#include <mlir/Analysis/TopologicalSortUtils.h>
 #include <mlir/Dialect/Arith/IR/Arith.h>
 
 #include <numeric>
@@ -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<mlir::Value> &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<evaluate::ArrayRef>(&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<mlir::arith::CmpIOp>(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<mlir::Value> 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<mlir::omp::MapBoundsOp>(
+ 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<mlir::Value> 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<fir::CoordinateOp>(
  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<mlir::Value> 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<mlir::omp::MapBoundsOp>(
- 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<fir::LoadOp>(clauseLocation, curValue);
@@ -473,50 +473,22 @@ void insertChildMapInfoIntoParent(
  if (parentExists) {
  auto mapOp = llvm::cast<mlir::omp::MapInfoOp>(
  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());
 
  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<mlir::Value> 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<mlir::omp::MapBoundsOp>(
- 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<mlir::Value> 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<mlir::Value> 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<mlir::omp::MapBoundsOp>(
- 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());

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<omp::MapBoundsOp>(
@@ -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

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