[Flang][OpenMP] Move handling from MLIR - LLVM-IR layer to FIR -> LLVM dialect layer

Likely a more palatable way to handle this for upstreaming.

Author: agozillon

flang/lib/Optimizer/CodeGen/CodeGenOpenMP.cpp

@@ -59,6 +59,21 @@ struct MapInfoOpConversion
     : public OpenMPFIROpConversion<mlir::omp::MapInfoOp> {
   using OpenMPFIROpConversion::OpenMPFIROpConversion;
 
+  mlir::omp::MapBoundsOp
+  createBoundsForCharString(mlir::ConversionPatternRewriter &rewriter,
+                            unsigned int len, mlir::Location loc) const {
+    mlir::Type i64Ty = rewriter.getIntegerType(64);
+    auto lBound = mlir::LLVM::ConstantOp::create(rewriter, loc, i64Ty, 0);
+    auto uBoundAndExt =
+        mlir::LLVM::ConstantOp::create(rewriter, loc, i64Ty, len - 1);
+    auto stride = mlir::LLVM::ConstantOp::create(rewriter, loc, i64Ty, 1);
+    auto baseLb = mlir::LLVM::ConstantOp::create(rewriter, loc, i64Ty, 1);
+    auto mapBoundType = rewriter.getType<mlir::omp::MapBoundsType>();
+    return mlir::omp::MapBoundsOp::create(rewriter, loc, mapBoundType, lBound,
+                                          uBoundAndExt, uBoundAndExt, stride,
+                                          /*strideInBytes*/ false, baseLb);
+  }
+
   llvm::LogicalResult
   matchAndRewrite(mlir::omp::MapInfoOp curOp, OpAdaptor adaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
@@ -68,13 +83,58 @@ struct MapInfoOpConversion
       return mlir::failure();
 
     llvm::SmallVector<mlir::NamedAttribute> newAttrs;
-    mlir::omp::MapInfoOp newOp;
+    mlir::omp::MapBoundsOp mapBoundsOp;
     for (mlir::NamedAttribute attr : curOp->getAttrs()) {
       if (auto typeAttr = mlir::dyn_cast<mlir::TypeAttr>(attr.getValue())) {
         mlir::Type newAttr;
         if (fir::isTypeWithDescriptor(typeAttr.getValue())) {
           newAttr = lowerTy().convertBoxTypeAsStruct(
               mlir::cast<fir::BaseBoxType>(typeAttr.getValue()));
+        } else if (fir::isa_char_string(fir::unwrapSequenceType(
+                       fir::unwrapPassByRefType(typeAttr.getValue()))) &&
+                   !characterWithDynamicLen(
+                       fir::unwrapPassByRefType(typeAttr.getValue()))) {
+          // Characters with a LEN param are represented as char
+          // arrays/strings, the initial lowering doesn't generate
+          // bounds for these, however, we require them to map the
+          // data appropriately in the later lowering stages. This
+          // is to prevent the need for unecessary caveats
+          // specific to Flang. We also strip the array from the
+          // type so that all variations of strings are treated
+          // identically and there's no caveats or specialisations
+          // required in the later stages. As an example, Boxed
+          // char strings will emit a single char array no matter
+          // the number of dimensions caused by additional array
+          // dimensions which needs specialised for, as it differs
+          // from the non-box variation which will emit each array
+          // wrapping the character array, e.g. given a type of
+          // the same dimensions, if one is boxed, the types would
+          // end up:
+          //
+          //     array<i8 x 16>
+          //  vs
+          //     array<10 x array< 10 x array<i8 x 16>>>
+          //
+          // This means we have to treat one specially in the
+          // lowering. So we try to "canonicalize" it here.
+          // TODO: Handle dynamic LEN characters.
+          if (auto ct = mlir::dyn_cast_or_null<fir::CharacterType>(
+                  fir::unwrapSequenceType(typeAttr.getValue()))) {
+            newAttr = converter->convertType(
+                fir::unwrapSequenceType(typeAttr.getValue()));
+            if (auto type = mlir::dyn_cast<mlir::LLVM::LLVMArrayType>(newAttr))
+              newAttr = type.getElementType();
+            // We do not generate for device, as MapBoundsOps are
+            // unsupported, as they're currently unused.
+            auto offloadMod =
+                llvm::dyn_cast_or_null<mlir::omp::OffloadModuleInterface>(
+                    *curOp->getParentOfType<mlir::ModuleOp>());
+            if (!offloadMod.getIsTargetDevice())
+              mapBoundsOp = createBoundsForCharString(rewriter, ct.getLen(),
+                                                      curOp.getLoc());
+          } else {
+            newAttr = converter->convertType(typeAttr.getValue());
+          }
         } else {
           newAttr = converter->convertType(typeAttr.getValue());
         }
@@ -84,8 +144,13 @@ struct MapInfoOpConversion
       }
     }
 
-    rewriter.replaceOpWithNewOp<mlir::omp::MapInfoOp>(
+    auto newOp = rewriter.replaceOpWithNewOp<mlir::omp::MapInfoOp>(
         curOp, resTypes, adaptor.getOperands(), newAttrs);
+    if (mapBoundsOp) {
+      rewriter.startOpModification(newOp);
+      newOp.getBoundsMutable().append(mlir::ValueRange{mapBoundsOp});
+      rewriter.finalizeOpModification(newOp);
+    }
 
     return mlir::success();
   }

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

@@ -3532,36 +3532,13 @@ static TargetDirective getTargetDirectiveFromOp(Operation *op) {
 
 } // namespace
 
-// In certain cases, we can be provided less bounds than there are nested array
-// types, but still be provided bounds, in these cases we try to compute the
-// size up to the point of the bounds provided and then let the bounds x size
-// computation do the rest of the work. This is most common in Flang where
-// character arrays provided character lengths (C/C++ string esque), represent
-// the internal string as a byte array with the length of this string
-// unrepresented by bounds.
-uint64_t getArrayElementSizeInBits(LLVM::LLVMArrayType arrTy, DataLayout &dl,
-                                   int boundsCount) {
-  if (boundsCount == 0)
-    return dl.getTypeSizeInBits(arrTy);
+uint64_t getArrayElementSizeInBits(LLVM::LLVMArrayType arrTy, DataLayout &dl) {
   if (auto nestedArrTy = llvm::dyn_cast_if_present<LLVM::LLVMArrayType>(
           arrTy.getElementType()))
-    return getArrayElementSizeInBits(nestedArrTy, dl, --boundsCount);
+    return getArrayElementSizeInBits(nestedArrTy, dl);
   return dl.getTypeSizeInBits(arrTy.getElementType());
 }
 
-// It is possible for a 1-D array type to provide N-D bounds to index
-// with instead of 1-D Bounds. this is common to do with byte arrays
-// that are representing other data types, e.g. an N-D char array, we
-// support this use case.
-// TODO: Extend to just check if we have more bounds than array
-// dimensions
-static bool is1DArrayWithNDBounds(llvm::Type *type, size_t numBounds) {
-  if (type->isArrayTy() && !type->getArrayElementType()->isArrayTy() &&
-      numBounds > 1)
-    return true;
-  return false;
-}
-
 // This function calculates the size to be offloaded for a specified type, given
 // its associated map clause (which can contain bounds information which affects
 // the total size), this size is calculated based on the underlying element type
@@ -3607,10 +3584,7 @@ llvm::Value *getSizeInBytes(DataLayout &dl, const mlir::Type &type,
       // the size in inconsistent byte or bit format.
       uint64_t underlyingTypeSzInBits = dl.getTypeSizeInBits(type);
       if (auto arrTy = llvm::dyn_cast_if_present<LLVM::LLVMArrayType>(type))
-        if (!is1DArrayWithNDBounds(moduleTranslation.convertType(type),
-                                   memberClause.getBounds().size()))
-          underlyingTypeSzInBits = getArrayElementSizeInBits(
-              arrTy, dl, memberClause.getBounds().size());
+        underlyingTypeSzInBits = getArrayElementSizeInBits(arrTy, dl);
 
       // The size in bytes x number of elements, the sizeInBytes stored is
       // the underyling types size, e.g. if ptr<i32>, it'll be the i32's
@@ -4443,10 +4417,7 @@ createAlteredByCaptureMap(MapInfoData &mapData,
       case omp::VariableCaptureKind::ByRef: {
         llvm::Value *newV = mapData.Pointers[i];
         std::vector<llvm::Value *> offsetIdx = calculateBoundsOffset(
-            moduleTranslation, builder,
-            mapData.BaseType[i]->isArrayTy() &&
-                !is1DArrayWithNDBounds(mapData.BaseType[i],
-                                       mapOp.getBounds().size()),
+            moduleTranslation, builder, mapData.BaseType[i]->isArrayTy(),
             mapOp.getBounds());
         if (isPtrTy)
           newV = builder.CreateLoad(builder.getPtrTy(), newV);