[Flang][OpenMP] Move char box bounds generation for Maps to DirectiveCommons.h

[agozillon]

Currently we generate these bounds in the MapInfoFinalization.cpp pass as it seems there's a missing case for character strings/arrays (length parameter) in the DirectiveCommons bounds generation functionality OpenMP uses for it's map operations.

This PR tries to add this case to the DirectiveCommons function and remove the need for the bounds generation in the MapInfoFinalization pass, so that we are generating the bounds in the same place most other bounds are generated.

[llvmbot]

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

Author: None (agozillon)

Changes

Currently we generate these bounds in the MapInfoFinalization.cpp pass as it seems there's a missing case for character strings/arrays (length parameter) in the DirectiveCommons bounds generation functionality OpenMP uses for it's map operations.

This PR tries to add this case to the DirectiveCommons function and remove the need for the bounds generation in the MapInfoFinalization pass, so that we are generating the bounds in the same place most other bounds are generated.

---

Full diff: https://github.com/llvm/llvm-project/pull/165918.diff

2 Files Affected:

• (modified) flang/include/flang/Lower/DirectivesCommon.h (+10-2)
• (modified) flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp (-32)

diff --git a/flang/include/flang/Lower/DirectivesCommon.h b/flang/include/flang/Lower/DirectivesCommon.h
index 2d6906738773a..b564ee1f64423 100644
--- a/flang/include/flang/Lower/DirectivesCommon.h
+++ b/flang/include/flang/Lower/DirectivesCommon.h
@@ -512,11 +512,19 @@ fir::factory::AddrAndBoundsInfo gatherDataOperandAddrAndBounds(
       }
       bool dataExvIsAssumedSize =
           Fortran::semantics::IsAssumedSizeArray(symRef->get().GetUltimate());
-      if (genDefaultBounds &&
-          mlir::isa<fir::SequenceType>(fir::unwrapRefType(info.addr.getType())))
+      if (genDefaultBounds && mlir::isa<fir::SequenceType>(
+                                  fir::unwrapRefType(info.addr.getType()))) {
         bounds = fir::factory::genBaseBoundsOps<BoundsOp, BoundsType>(
             builder, operandLocation, dataExv, dataExvIsAssumedSize,
             strideIncludeLowerExtent);
+      }
+      if (genDefaultBounds && fir::characterWithDynamicLen(
+                                  fir::unwrapRefType(info.addr.getType())) ||
+          mlir::isa<fir::BoxCharType>(
+              fir::unwrapRefType(info.addr.getType()))) {
+        bounds = {fir::factory::genBoundsOpFromBoxChar<BoundsOp, BoundsType>(
+            builder, operandLocation, dataExv, info)};
+      }
       asFortran << symRef->get().name().ToString();
     } else { // Unsupported
       llvm::report_fatal_error("Unsupported type of OpenACC operand");
diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
index bd07d7fe01b85..32faada7a3b49 100644
--- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
+++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
@@ -987,38 +987,6 @@ class MapInfoFinalizationPass
       localBoxAllocas.clear();
       deferrableDesc.clear();
 
-      // First, walk `omp.map.info` ops to see if any of them have varPtrs
-      // with an underlying type of fir.char<k, ?>, i.e a character
-      // with dynamic length. If so, check if they need bounds added.
-      func->walk([&](mlir::omp::MapInfoOp op) {
-        if (!op.getBounds().empty())
-          return;
-
-        mlir::Value varPtr = op.getVarPtr();
-        mlir::Type underlyingVarType = fir::unwrapRefType(varPtr.getType());
-
-        if (!fir::characterWithDynamicLen(underlyingVarType))
-          return;
-
-        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},
-                                            /*continguousHint=*/true)
-                .first;
-        builder.setInsertionPoint(op);
-        llvm::SmallVector<mlir::Value> boundsOps =
-            fir::factory::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
-                                               mlir::omp::MapBoundsType>(
-                builder, info, extendedValue,
-                /*dataExvIsAssumedSize=*/false, varPtr.getLoc());
-
-        op.getBoundsMutable().append(boundsOps);
-      });
-
       // Next, walk `omp.map.info` ops to see if any record members should be
       // implicitly mapped.
       func->walk([&](mlir::omp::MapInfoOp op) {

[bhandarkar-pranav]

LGTM, thank you @agozillon

[agozillon]

Thank you @bhandarkar-pranav for the quick review! I'll give it until the middle of next week before landing the PR incase @vzakhari or @razvanlupusoru have any input as we share the same infrastructure for this with OpenACC!

[razvanlupusoru]

Thank you for this fix! Is there an associated test case that you could include?

[razvanlupusoru]

Marking as approved as I will be PTO shortly and won't get another chance to look at this. Please add a test case before final submission. Thank you!

[agozillon]

Thank you very much for the quick review @razvanlupusoru I'll add a test next week before I land the PR! I hope you have a good time on your PTO!