Skip to content

[Flang][OpenMP] - When mapping a fir.boxchar, map the underlying data pointer as a member #141715

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

bhandarkar-pranav
Copy link
Contributor

@bhandarkar-pranav bhandarkar-pranav commented May 28, 2025

This PR adds functionality to the MapInfoFinalization pass wherein the underlying data pointer of a fir.boxchar is mapped as a member of the parent boxchar.

PR Stack

@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp labels May 28, 2025
@llvmbot
Copy link
Member

llvmbot commented May 28, 2025

@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-fir-hlfir

Author: Pranav Bhandarkar (bhandarkar-pranav)

Changes

This PR adds functionality to the MapInfoFinalization pass wherein the underlying data pointer of a fir.boxchar is mapped as a member of the parent boxchar.


Patch is 35.72 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/141715.diff

8 Files Affected:

  • (modified) flang/include/flang/Optimizer/Builder/DirectivesCommon.h (+65-20)
  • (modified) flang/lib/Optimizer/Dialect/FIRType.cpp (+3)
  • (modified) flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp (+87-1)
  • (modified) flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp (+41-26)
  • (modified) flang/test/Fir/convert-to-llvm-openmp-and-fir.fir (+27)
  • (modified) flang/test/Lower/OpenMP/map-character.f90 (+18-5)
  • (modified) flang/test/Lower/OpenMP/optional-argument-map-2.f90 (+56-7)
  • (modified) flang/test/Transforms/omp-map-info-finalization.fir (+59)
diff --git a/flang/include/flang/Optimizer/Builder/DirectivesCommon.h b/flang/include/flang/Optimizer/Builder/DirectivesCommon.h
index 3f30c761acb4e..be11b9b5ede7c 100644
--- a/flang/include/flang/Optimizer/Builder/DirectivesCommon.h
+++ b/flang/include/flang/Optimizer/Builder/DirectivesCommon.h
@@ -91,6 +91,16 @@ inline AddrAndBoundsInfo getDataOperandBaseAddr(fir::FirOpBuilder &builder,
 
     return AddrAndBoundsInfo(symAddr, rawInput, isPresent, boxTy);
   }
+  // For boxchar references, do the same as what is done above for box
+  // references - Load the boxchar so that it is easier to retrieve the length
+  // of the underlying character and the data pointer.
+  if (auto boxCharType = mlir::dyn_cast<fir::BoxCharType>(
+          fir::unwrapRefType((symAddr.getType())))) {
+    if (!isOptional && mlir::isa<fir::ReferenceType>(symAddr.getType())) {
+      mlir::Value boxChar = builder.create<fir::LoadOp>(loc, symAddr);
+      return AddrAndBoundsInfo(boxChar, rawInput, isPresent);
+    }
+  }
   return AddrAndBoundsInfo(symAddr, rawInput, isPresent);
 }
 
@@ -137,26 +147,61 @@ template <typename BoundsOp, typename BoundsType>
 mlir::Value
 genBoundsOpFromBoxChar(fir::FirOpBuilder &builder, mlir::Location loc,
                        fir::ExtendedValue dataExv, AddrAndBoundsInfo &info) {
-  // TODO: Handle info.isPresent.
-  if (auto boxCharType =
-          mlir::dyn_cast<fir::BoxCharType>(info.addr.getType())) {
-    mlir::Type idxTy = builder.getIndexType();
-    mlir::Type lenType = builder.getCharacterLengthType();
+
+  if (!mlir::isa<fir::BoxCharType>(fir::unwrapRefType(info.addr.getType())))
+    return mlir::Value{};
+
+  mlir::Type idxTy = builder.getIndexType();
+  mlir::Type lenType = builder.getCharacterLengthType();
+  mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0);
+  mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1);
+  using ExtentAndStride = std::tuple<mlir::Value, mlir::Value>;
+  auto [extent, stride] = [&]() -> ExtentAndStride {
+    if (info.isPresent) {
+      llvm::SmallVector<mlir::Type> resTypes = {idxTy, idxTy};
+      mlir::Operation::result_range ifRes =
+          builder.genIfOp(loc, resTypes, info.isPresent, /*withElseRegion=*/true)
+              .genThen([&]() {
+                mlir::Value boxChar =
+                    fir::isa_ref_type(info.addr.getType())
+                        ? builder.create<fir::LoadOp>(loc, info.addr)
+                        : info.addr;
+                fir::BoxCharType boxCharType =
+                    mlir::cast<fir::BoxCharType>(boxChar.getType());
+                mlir::Type refType = builder.getRefType(boxCharType.getEleTy());
+                auto unboxed = builder.create<fir::UnboxCharOp>(
+                    loc, refType, lenType, boxChar);
+                mlir::SmallVector<mlir::Value> results = {unboxed.getResult(1), one };
+                builder.create<fir::ResultOp>(loc, results);
+              })
+          .genElse([&]() {
+            mlir::SmallVector<mlir::Value> results = {zero, zero };
+            builder.create<fir::ResultOp>(loc, results); })
+          .getResults();
+      return {ifRes[0], ifRes[1]};
+    }
+    // We have already established that info.addr.getType() is a boxchar
+    // or a boxchar address. If an address, load the boxchar.
+    mlir::Value boxChar = fir::isa_ref_type(info.addr.getType())
+                              ? builder.create<fir::LoadOp>(loc, info.addr)
+                              : info.addr;
+    fir::BoxCharType boxCharType =
+        mlir::cast<fir::BoxCharType>(boxChar.getType());
     mlir::Type refType = builder.getRefType(boxCharType.getEleTy());
     auto unboxed =
-        builder.create<fir::UnboxCharOp>(loc, refType, lenType, info.addr);
-    mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0);
-    mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1);
-    mlir::Value extent = unboxed.getResult(1);
-    mlir::Value stride = one;
-    mlir::Value ub = builder.create<mlir::arith::SubIOp>(loc, extent, one);
-    mlir::Type boundTy = builder.getType<mlir::omp::MapBoundsType>();
-    return builder.create<mlir::omp::MapBoundsOp>(
-        loc, boundTy, /*lower_bound=*/zero,
-        /*upper_bound=*/ub, /*extent=*/extent, /*stride=*/stride,
-        /*stride_in_bytes=*/true, /*start_idx=*/zero);
-  }
-  return mlir::Value{};
+        builder.create<fir::UnboxCharOp>(loc, refType, lenType, boxChar);
+    return {unboxed.getResult(1), one};
+  }();
+
+  mlir::Value ub = builder.create<mlir::arith::SubIOp>(loc, extent, one);
+  mlir::Type boundTy = builder.getType<BoundsType>();
+  return builder.create<BoundsOp>(loc, boundTy,
+                                  /*lower_bound=*/zero,
+                                  /*upper_bound=*/ub,
+                                  /*extent=*/extent,
+                                  /*stride=*/stride,
+                                  /*stride_in_bytes=*/true,
+                                  /*start_idx=*/zero);
 }
 
 /// Generate the bounds operation from the descriptor information.
@@ -296,11 +341,11 @@ genImplicitBoundsOps(fir::FirOpBuilder &builder, AddrAndBoundsInfo &info,
     bounds = genBaseBoundsOps<BoundsOp, BoundsType>(builder, loc, dataExv,
                                                     dataExvIsAssumedSize);
   }
-  if (characterWithDynamicLen(fir::unwrapRefType(baseOp.getType()))) {
+  if (characterWithDynamicLen(fir::unwrapRefType(baseOp.getType())) ||
+      mlir::isa<fir::BoxCharType>(fir::unwrapRefType(info.addr.getType()))) {
     bounds = {genBoundsOpFromBoxChar<BoundsOp, BoundsType>(builder, loc,
                                                            dataExv, info)};
   }
-
   return bounds;
 }
 
diff --git a/flang/lib/Optimizer/Dialect/FIRType.cpp b/flang/lib/Optimizer/Dialect/FIRType.cpp
index da7aa17445404..f521c9b3058fd 100644
--- a/flang/lib/Optimizer/Dialect/FIRType.cpp
+++ b/flang/lib/Optimizer/Dialect/FIRType.cpp
@@ -285,6 +285,9 @@ bool hasDynamicSize(mlir::Type t) {
     return true;
   if (auto rec = mlir::dyn_cast<fir::RecordType>(t))
     return hasDynamicSize(rec);
+  if (auto boxChar = mlir::dyn_cast<fir::BoxCharType>(t)) {
+    return characterWithDynamicLen(boxChar.getEleTy());
+  }
   return false;
 }
 
diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
index 86095d708f7e5..51237840f060e 100644
--- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
+++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
@@ -48,7 +48,7 @@
 #include <numeric>
 
 #define DEBUG_TYPE "omp-map-info-finalization"
-#define PDBGS() (llvm::dbgs() << "[" << DEBUG_TYPE << "]: ")
+
 namespace flangomp {
 #define GEN_PASS_DEF_MAPINFOFINALIZATIONPASS
 #include "flang/Optimizer/OpenMP/Passes.h.inc"
@@ -285,6 +285,60 @@ class MapInfoFinalizationPass
     return false;
   }
 
+  mlir::omp::MapInfoOp genBoxcharMemberMap(mlir::omp::MapInfoOp op,
+                                           fir::FirOpBuilder &builder) {
+    if (!op.getMembers().empty())
+      return op;
+    mlir::Location loc = op.getVarPtr().getLoc();
+    mlir::Value boxChar = op.getVarPtr();
+
+    if (mlir::isa<fir::ReferenceType>(op.getVarPtr().getType()))
+      boxChar = builder.create<fir::LoadOp>(loc, op.getVarPtr());
+
+    fir::BoxCharType boxCharType = mlir::dyn_cast<fir::BoxCharType>(boxChar.getType());
+    mlir::Value boxAddr = builder.create<fir::BoxOffsetOp>(loc, op.getVarPtr(), fir::BoxFieldAttr::base_addr);
+
+    uint64_t mapTypeToImplicit = static_cast<
+        std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>(
+        llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO |
+        llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_IMPLICIT);
+
+    mlir::ArrayAttr  newMembersAttr;
+    llvm::SmallVector<llvm::SmallVector<int64_t>> memberIdx = {{0}};
+    newMembersAttr = builder.create2DI64ArrayAttr(memberIdx);
+
+    mlir::Value varPtr = op.getVarPtr();
+    mlir::omp::MapInfoOp memberMapInfoOp = builder.create<mlir::omp::MapInfoOp>(
+        op.getLoc(), varPtr.getType(), varPtr,
+        mlir::TypeAttr::get(boxCharType.getEleTy()),
+        builder.getIntegerAttr(builder.getIntegerType(64, /*isSigned=*/false),
+                               mapTypeToImplicit),
+        builder.getAttr<mlir::omp::VariableCaptureKindAttr>(
+            mlir::omp::VariableCaptureKind::ByRef),
+        /*varPtrPtr=*/boxAddr,
+        /*members=*/llvm::SmallVector<mlir::Value>{},
+        /*member_index=*/mlir::ArrayAttr{},
+        /*bounds=*/op.getBounds(),
+        /*mapperId=*/mlir::FlatSymbolRefAttr(), /*name=*/op.getNameAttr(),
+        builder.getBoolAttr(false));
+
+    mlir::omp::MapInfoOp newMapInfoOp = builder.create<mlir::omp::MapInfoOp>(
+        op.getLoc(), op.getResult().getType(), varPtr,
+        mlir::TypeAttr::get(llvm::cast<mlir::omp::PointerLikeType>(varPtr.getType())
+                            .getElementType()),
+        op.getMapTypeAttr(),
+        op.getMapCaptureTypeAttr(),
+        /*varPtrPtr=*/mlir::Value{},
+        /*members=*/llvm::SmallVector<mlir::Value>{memberMapInfoOp},
+        /*member_index=*/newMembersAttr,
+        /*bounds=*/llvm::SmallVector<mlir::Value>{},
+        /*mapperId=*/mlir::FlatSymbolRefAttr(), op.getNameAttr(),
+        /*partial_map=*/builder.getBoolAttr(false));
+    op.replaceAllUsesWith(newMapInfoOp.getResult());
+    op->erase();
+    return newMapInfoOp;
+  }
+
   mlir::omp::MapInfoOp genDescriptorMemberMaps(mlir::omp::MapInfoOp op,
                                                fir::FirOpBuilder &builder,
                                                mlir::Operation *target) {
@@ -575,6 +629,7 @@ class MapInfoFinalizationPass
         fir::factory::AddrAndBoundsInfo info =
             fir::factory::getDataOperandBaseAddr(
                 builder, varPtr, /*isOptional=*/false, varPtr.getLoc());
+
         fir::ExtendedValue extendedValue =
             hlfir::translateToExtendedValue(varPtr.getLoc(), builder,
                                             hlfir::Entity{info.addr},
@@ -743,6 +798,37 @@ class MapInfoFinalizationPass
         return mlir::WalkResult::advance();
       });
 
+      func->walk([&](mlir::omp::MapInfoOp op) {
+        if (!op.getMembers().empty())
+          return;
+
+        if (!mlir::isa<fir::BoxCharType>(fir::unwrapRefType(op.getVarType())))
+          return;
+
+        // POSSIBLE_HACK_ALERT: If the boxchar has been implicitly mapped then
+        // it is likely that the underlying pointer to the data
+        // (!fir.ref<fir.char<k,?>>) has already been mapped. So, skip such
+        // boxchars. We are primarily interested in boxchars that were mapped
+        // by passes such as MapsForPrivatizedSymbols that map boxchars that
+        // are privatized. At present, such boxchar maps are not marked
+        // implicit. Should they be? I don't know. If they should be then
+        // we need to change this check for early return OR live with
+        // over-mapping.
+        bool hasImplicitMap =
+            (llvm::omp::OpenMPOffloadMappingFlags(op.getMapType()) &
+             llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_IMPLICIT) ==
+            llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_IMPLICIT;
+        if (hasImplicitMap)
+          return;
+
+        assert(llvm::hasSingleElement(op->getUsers()) &&
+               "OMPMapInfoFinalization currently only supports single users "
+               "of a MapInfoOp");
+
+        builder.setInsertionPoint(op);
+        genBoxcharMemberMap(op, builder);
+      });
+
       func->walk([&](mlir::omp::MapInfoOp op) {
         // TODO: Currently only supports a single user for the MapInfoOp. This
         // is fine for the moment, as the Fortran frontend will generate a
diff --git a/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp b/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp
index dedff59d66aa0..70f051789acb7 100644
--- a/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp
+++ b/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp
@@ -22,7 +22,9 @@
 // 2. Generalize this for more than just omp.target ops.
 //===----------------------------------------------------------------------===//
 
+#include "flang/Optimizer/Builder/DirectivesCommon.h"
 #include "flang/Optimizer/Builder/FIRBuilder.h"
+#include "flang/Optimizer/Builder/HLFIRTools.h"
 #include "flang/Optimizer/Dialect/FIRType.h"
 #include "flang/Optimizer/Dialect/Support/KindMapping.h"
 #include "flang/Optimizer/HLFIR/HLFIROps.h"
@@ -188,32 +190,45 @@ class MapsForPrivatizedSymbolsPass
   // in a subsequent PR.
   void genBoundsOps(fir::FirOpBuilder &builder, mlir::Value var,
                     llvm::SmallVector<mlir::Value> &boundsOps) {
-    if (!fir::isBoxAddress(var.getType()))
-      return;
-
-    unsigned int rank = 0;
-    rank = fir::getBoxRank(fir::unwrapRefType(var.getType()));
-    mlir::Location loc = var.getLoc();
-    mlir::Type idxTy = builder.getIndexType();
-    mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1);
-    mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0);
-    mlir::Type boundTy = builder.getType<omp::MapBoundsType>();
-    mlir::Value box = builder.create<fir::LoadOp>(loc, var);
-    for (unsigned int i = 0; i < rank; ++i) {
-      mlir::Value dimNo = builder.createIntegerConstant(loc, idxTy, i);
-      auto dimInfo =
-          builder.create<fir::BoxDimsOp>(loc, idxTy, idxTy, idxTy, box, dimNo);
-      mlir::Value lb = dimInfo.getLowerBound();
-      mlir::Value extent = dimInfo.getExtent();
-      mlir::Value byteStride = dimInfo.getByteStride();
-      mlir::Value ub = builder.create<mlir::arith::SubIOp>(loc, extent, one);
-
-      mlir::Value boundsOp = builder.create<omp::MapBoundsOp>(
-          loc, boundTy, /*lower_bound=*/zero,
-          /*upper_bound=*/ub, /*extent=*/extent, /*stride=*/byteStride,
-          /*stride_in_bytes = */ true, /*start_idx=*/lb);
-      LLVM_DEBUG(PDBGS() << "Created BoundsOp " << boundsOp << "\n");
-      boundsOps.push_back(boundsOp);
+    if (fir::isBoxAddress(var.getType())) {
+      unsigned int rank = 0;
+      rank = fir::getBoxRank(fir::unwrapRefType(var.getType()));
+      mlir::Location loc = var.getLoc();
+      mlir::Type idxTy = builder.getIndexType();
+      mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1);
+      mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0);
+      mlir::Type boundTy = builder.getType<omp::MapBoundsType>();
+      mlir::Value box = builder.create<fir::LoadOp>(loc, var);
+      for (unsigned int i = 0; i < rank; ++i) {
+        mlir::Value dimNo = builder.createIntegerConstant(loc, idxTy, i);
+        auto dimInfo = builder.create<fir::BoxDimsOp>(loc, idxTy, idxTy, idxTy,
+                                                      box, dimNo);
+        mlir::Value lb = dimInfo.getLowerBound();
+        mlir::Value extent = dimInfo.getExtent();
+        mlir::Value byteStride = dimInfo.getByteStride();
+        mlir::Value ub = builder.create<mlir::arith::SubIOp>(loc, extent, one);
+
+        mlir::Value boundsOp = builder.create<omp::MapBoundsOp>(
+            loc, boundTy, /*lower_bound=*/zero,
+            /*upper_bound=*/ub, /*extent=*/extent, /*stride=*/byteStride,
+            /*stride_in_bytes = */ true, /*start_idx=*/lb);
+        LLVM_DEBUG(PDBGS() << "Created BoundsOp " << boundsOp << "\n");
+        boundsOps.push_back(boundsOp);
+      }
+    } else {
+      mlir::Location loc = var.getLoc();
+      fir::factory::AddrAndBoundsInfo info = fir::factory::getDataOperandBaseAddr(builder, var, /*isOptional=*/false, loc);
+      fir::ExtendedValue extendedValue =
+          hlfir::translateToExtendedValue(loc, builder,
+                                          hlfir::Entity{info.addr},
+                                          /*continguousHint=*/true).first;
+      llvm::SmallVector<mlir::Value> boundsOpsVec =
+          fir::factory::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
+                                             mlir::omp::MapBoundsType>(
+                builder, info, extendedValue,
+                /*dataExvIsAssumedSize=*/false, loc);
+      for (auto bounds : boundsOpsVec)
+        boundsOps.push_back(bounds);
     }
   }
 };
diff --git a/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir b/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
index b13921f822b4d..24e5cad84b709 100644
--- a/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
+++ b/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
@@ -1288,3 +1288,30 @@ omp.declare_mapper @my_mapper : !fir.type<_QFdeclare_mapperTmy_type{data:i32}> {
   omp.declare_mapper.info map_entries(%4, %3 : !fir.ref<!fir.type<_QFdeclare_mapperTmy_type{data:i32}>>, !fir.ref<i32>)
 // CHECK:         }
 }
+
+// -----
+omp.private {type = firstprivate} @boxchar_privatizer : !fir.boxchar<1> copy {
+  ^bb0(%arg0: !fir.boxchar<1>, %arg1: !fir.boxchar<1>):
+    omp.yield(%arg1 : !fir.boxchar<1>)
+}
+
+func.func @map_privatized_boxchar(%arg0 : !fir.boxchar<1>) {
+    %0 = fir.alloca !fir.boxchar<1>
+    fir.store %arg0 to %0 : !fir.ref<!fir.boxchar<1>>
+    %7 = fir.box_offset %0 base_addr : (!fir.ref<!fir.boxchar<1>>) -> !fir.llvm_ptr<!fir.ref<!fir.char<1,?>>>
+    %8 = omp.map.info var_ptr(%0 : !fir.ref<!fir.boxchar<1>>, !fir.char<1,?>) map_clauses(implicit, to) capture(ByRef) var_ptr_ptr(%7 : !fir.llvm_ptr<!fir.ref<!fir.char<1,?>>>) -> !fir.ref<!fir.boxchar<1>>
+    %9 = omp.map.info var_ptr(%0 : !fir.ref<!fir.boxchar<1>>, !fir.boxchar<1>) map_clauses(to) capture(ByRef) members(%8 : [0] : !fir.ref<!fir.boxchar<1>>) -> !fir.ref<!fir.boxchar<1>>
+    omp.target map_entries(%9 -> %arg1, %8 -> %arg2 : !fir.ref<!fir.boxchar<1>>, !fir.ref<!fir.boxchar<1>>) private(@boxchar_privatizer %arg0 -> %arg3 [map_idx=0] : !fir.boxchar<1>) {
+      omp.terminator
+    }
+    return
+}
+
+// CHECK-LABEL: llvm.func @map_privatized_boxchar(
+// CHECK-SAME: %[[ARG0:.*]]: !llvm.struct<(ptr, i64)>) {
+// CHECK: %[[BOXCHAR_ALLOCA:.*]] = llvm.alloca {{.*}} x !llvm.struct<(ptr, i64)> : (i64) -> !llvm.ptr
+// CHECK: llvm.store %[[ARG0]], %[[BOXCHAR_ALLOCA]] : !llvm.struct<(ptr, i64)>, !llvm.ptr
+// CHECK: %[[BASE_ADDR:.*]] = llvm.getelementptr %[[BOXCHAR_ALLOCA]][0, 0] : (!llvm.ptr) -> !llvm.ptr, !llvm.struct<(ptr, i64)>
+// CHECK: %[[MAP_BASE_ADDR:.*]] = omp.map.info var_ptr(%[[BOXCHAR_ALLOCA]] : !llvm.ptr, i8) map_clauses(implicit, to) capture(ByRef) var_ptr_ptr(%[[BASE_ADDR]] : !llvm.ptr) -> !llvm.ptr
+// CHECK: %[[MAP_BOXCHAR:.*]] = omp.map.info var_ptr(%[[BOXCHAR_ALLOCA]] : !llvm.ptr, !llvm.struct<(ptr, i64)>) map_clauses(to) capture(ByRef) members(%[[MAP_BASE_ADDR]] : [0] : !llvm.ptr) -> !llvm.ptr
+// CHECK: omp.target map_entries(%[[MAP_BOXCHAR]] -> %arg1, %[[MAP_BASE_ADDR]] -> %arg2 : !llvm.ptr, !llvm.ptr) private(@boxchar_privatizer %[[ARG0]] -> %arg3 [map_idx=0] : !llvm.struct<(ptr, i64)>) {
diff --git a/flang/test/Lower/OpenMP/map-character.f90 b/flang/test/Lower/OpenMP/map-character.f90
index 232c0a6361cb6..cefd3ac0e54f9 100644
--- a/flang/test/Lower/OpenMP/map-character.f90
+++ b/flang/test/Lower/OpenMP/map-character.f90
@@ -18,25 +18,38 @@ end subroutine TestOfCharacter
 !CHECK:  %[[A0_DECL:.*]]:2 = hlfir.declare %[[UNBOXED_ARG0]]#0 typeparams %[[UNBOXED_ARG0]]#1 dummy_scope {{.*}} -> (!fir.boxchar<1>, !fir.ref<!fir.char<1,?>>)
 !CHECK:  %[[UNBOXED_ARG1:.*]]:2 = fir.unboxchar %[[ARG1]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
 !CHECK:  %[[A1_DECL:.*]]:2 = hlfir.declare %[[UNBOXED_ARG1]]#0 typeparams %[[UNBOXED_ARG1]]#1 dummy_scope {{.*}} -> (!fir.boxchar<1>, !fir.ref<!fir.char<1,?>>)
-!CHECK:  %[[UNBOXED_A0_DECL:.*]]:2 = fir.unboxchar %[[A0_DECL]]#0 : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
 !CHECK:  %[[A0_LB:.*]] = arith.constant 0 : index
 !CHECK:  %[[A0_STRIDE:.*]] = arith.constant 1 : index
+!CHECK:  %[[UNBOXED_A0_DECL:.*]]:2 = fir.unboxchar %[[A0_DECL]]#0 : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
 !CHECK:  %[[A0_UB:.*]] = arith.subi %[[UNBOXED_A0_DECL]]#1, %[[A0_STRIDE]] : index
 !CHECK:  %[[A0_BOUNDS:.*]] = omp.map.bounds lower_bound(%[[A0_LB]] : index) upper_bound(%[[A0_UB]] : index) extent(%[[UNBOXED_A0_DECL]]#1 : index)
 !CHECK-SAME:  stride(%[[A0_STRIDE]] : index) start_idx(%[[A0_LB]] : index) {stride_in_bytes = true}
 !CHECK:  %[[A0_MAP:.*]] = omp.map.info var_ptr(%[[A0_DECL]]#1 : !fir.ref<!fir.char<1,?>>, !fir.char<1...
[truncated]

Copy link

github-actions bot commented May 28, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@bhandarkar-pranav bhandarkar-pranav force-pushed the users/bhandarkar-pranav/boxchar_firstprivate_fix1 branch from bedb79e to 40db6c7 Compare May 28, 2025 15:33
@bhandarkar-pranav bhandarkar-pranav force-pushed the users/bhandarkar-pranav/boxchar_firstprivate_fix1 branch from f5de81e to 7fc7fea Compare June 3, 2025 18:25
Copy link
Contributor

@agozillon agozillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, just had one question that's all :-)

@@ -285,6 +285,62 @@ class MapInfoFinalizationPass
return false;
}

mlir::omp::MapInfoOp genBoxcharMemberMap(mlir::omp::MapInfoOp op,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a dumb question, but wouldn't this and the related walk already be handled by the larger: genDescriptorMemberMaps function? In theory it should be able to handle the expansion for all cases, I would imagine it'd need some tweaking in-cases where we assume BaseBoxType/BoxType though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @agozillon for the review. Yes, it was related to the fact that genDescriptorMemberMaps is tied quite tightly to BaseBoxType. So, I decided to not mess with it. I should add it to my to-do list though to generalize genDescriptorMemberMaps.

return;

// POSSIBLE_HACK_ALERT: If the boxchar has been implicitly mapped then
// it is likely that the underlying pointer to the data
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think as long as it's the same address, the runtime wouldn't necessarily invoke another data transferal, just bump the ref counter, so it wouldn't be too costly in these cases I think! But it is nice to avoid generating extra maps where feasible :-)

…lper is supposed

to return false for pointer-like types which boxchar is. Clients should use a "preprocessing"
helper such as dyn_cast_ptrOrBoxEleTy before calling hasDynamicSize as shown below

bool HasDynamicSize = fir::hasDynamicSize(fir::dyn_cast_ptrOrBoxEleTy(boxCharType));
@bhandarkar-pranav bhandarkar-pranav force-pushed the users/bhandarkar-pranav/boxchar_firstprivate_fix1 branch from a470a91 to d438e21 Compare June 5, 2025 17:01
Base automatically changed from users/bhandarkar-pranav/boxchar_firstprivate_fix0 to main June 6, 2025 15:48
Copy link
Contributor

@jeanPerier jeanPerier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comment. FIR changes LGTM.

@bhandarkar-pranav bhandarkar-pranav merged commit f993f36 into main Jun 10, 2025
11 checks passed
@bhandarkar-pranav bhandarkar-pranav deleted the users/bhandarkar-pranav/boxchar_firstprivate_fix1 branch June 10, 2025 18:09
rorth pushed a commit to rorth/llvm-project that referenced this pull request Jun 11, 2025
…ta pointer as a member (llvm#141715)

This PR adds functionality to the `MapInfoFinalization` pass wherein the
underlying data pointer of a `fir.boxchar` is mapped as a member of the
parent boxchar.
tomtor pushed a commit to tomtor/llvm-project that referenced this pull request Jun 14, 2025
…ta pointer as a member (llvm#141715)

This PR adds functionality to the `MapInfoFinalization` pass wherein the
underlying data pointer of a `fir.boxchar` is mapped as a member of the
parent boxchar.
akuhlens pushed a commit to akuhlens/llvm-project that referenced this pull request Jun 24, 2025
…ta pointer as a member (llvm#141715)

This PR adds functionality to the `MapInfoFinalization` pass wherein the
underlying data pointer of a `fir.boxchar` is mapped as a member of the
parent boxchar.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:fir-hlfir flang:openmp flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants