Skip to content

[Flang][OpenMP][MLIR] Initial derived type member map support #82853

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

Conversation

agozillon
Copy link
Contributor

@agozillon agozillon commented Feb 24, 2024

This patch is one in a stack of four patches that seeks to refactor
slightly and extend the current record type map support that was
put in place for Fortran's descriptor types to handle explicit
member mapping for record types at a single level of depth.

For example, the below case where members of Fortran
derived types are mapped explicitly:

  type :: scalar_and_array
    real(4) :: real
    integer(4) :: array(10)
    integer(4) :: int
  end type scalar_and_array
  type(scalar_and_array) :: scalar_arr

  !$omp target map(tofrom: scalar_arr%int, scalar_arr%real)
    type :: bottom_layer
      real(8) :: i2
      real(4) :: array_i2(10)
      real(4) :: array_j2(10)
    end type bottom_layer

    type :: top_layer
      real(4) :: i
      integer(4) :: array_i(10)
      real(4) :: j
      type(bottom_layer) :: nested
      integer, allocatable :: array_j(:)
      integer(4) :: k
    end type top_layer
    
    type(top_layer) :: top_dtype

!$omp target map(tofrom: top_dtype%nested%i2, top_dtype%k, top_dtype%nested%array_i2)

Current cases of derived type mapping left for future work are:

  • Fortran's automagical mapping of all elements and nested elements of a derived type
  • explicit member mapping of a derived type and then constituent members (redundant in Fortran due to former case but still legal as far as I am aware)
  • explicit member mapping of a record type (may be handled reasonably, just not fully tested in this iteration)
  • explicit member mapping for Fortran allocatable types (a variation of nested record types)

This patch seeks to support this by extending the Flang-new OpenMP lowering to
support generation of this newly required information, creating the necessary
parent <-to-> member map_info links, calculating the member indices and
setting if it's a partial map.

The OMPDescriptorMapInfoGen pass has also been generalized into a map
finalization phase, now named OMPMapInfoFinalization. This pass was extended
to support the insertion of member maps into the BlockArg and MapOperands of
relevant map carrying operations. Similar to the method in which descriptor types
are expanded and constituent members inserted.

Created using spr 1.3.4
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp labels Feb 24, 2024
@llvmbot
Copy link
Member

llvmbot commented Feb 24, 2024

@llvm/pr-subscribers-flang-openmp

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

Author: None (agozillon)

Changes

This patch is one in a series of four patches that seeks to refactor
slightly and extend the current record type map support that was
put in place for Fortran's descriptor types to handle explicit
member mapping for record types at a single level of depth.

For example, the below case where two members of a Fortran
derived type are mapped explicitly:

''''
type :: scalar_and_array
real(4) :: real
integer(4) :: array(10)
integer(4) :: int
end type scalar_and_array
type(scalar_and_array) :: scalar_arr

!$omp target map(tofrom: scalar_arr%int, scalar_arr%real)
''''

Current cases of derived type mapping left for future work are:
> explicit member mapping of nested members (e.g. two layers of
record types where we explicitly map a member from the internal
record type)
> Fortran's automagical mapping of all elements and nested elements
of a derived type
> explicit member mapping of a derived type and then constituient members
(redundant in Fortran due to former case but still legal as far as I am aware)
> explicit member mapping of a record type (may be handled reasonably, just
not fully tested in this iteration)
> explicit member mapping for Fortran allocatable types (a variation of nested
record types)

This patch seeks to support this by extending the Flang-new OpenMP lowering to
support generation of this newly required information, creating the neccessary
parent <-to-> member map_info links, calculating the member indices and
setting if it's a partial map.

The OMPDescriptorMapInfoGen pass has also been generalized into a map
finalization phase, now named OMPMapInfoFinalization. This pass was extended
to support the insertion of member maps into the BlockArg and MapOperands of
relevant map carrying operations. Similar to the method in which descriptor types
are expanded and constituient members inserted.


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

24 Files Affected:

  • (modified) flang/docs/OpenMP-descriptor-management.md (+2-2)
  • (modified) flang/include/flang/Optimizer/Transforms/Passes.h (+1-1)
  • (modified) flang/include/flang/Optimizer/Transforms/Passes.td (+3-3)
  • (modified) flang/include/flang/Tools/CLOptions.inc (+1-1)
  • (modified) flang/lib/Lower/OpenMP/ClauseProcessor.cpp (+50-18)
  • (modified) flang/lib/Lower/OpenMP/ClauseProcessor.h (+47-13)
  • (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+12-8)
  • (modified) flang/lib/Lower/OpenMP/Utils.cpp (+101-1)
  • (modified) flang/lib/Lower/OpenMP/Utils.h (+27-3)
  • (modified) flang/lib/Optimizer/Transforms/CMakeLists.txt (+1-1)
  • (removed) flang/lib/Optimizer/Transforms/OMPDescriptorMapInfoGen.cpp (-168)
  • (added) flang/lib/Optimizer/Transforms/OMPMapInfoFinalization.cpp (+264)
  • (modified) flang/test/Fir/convert-to-llvm-openmp-and-fir.fir (+31-4)
  • (modified) flang/test/Integration/OpenMP/map-types-and-sizes.f90 (+153-1)
  • (modified) flang/test/Lower/OpenMP/FIR/array-bounds.f90 (+4-4)
  • (modified) flang/test/Lower/OpenMP/FIR/map-component-ref.f90 (+2-2)
  • (modified) flang/test/Lower/OpenMP/FIR/target.f90 (+2-2)
  • (modified) flang/test/Lower/OpenMP/allocatable-array-bounds.f90 (+6-6)
  • (modified) flang/test/Lower/OpenMP/allocatable-map.f90 (+4-4)
  • (modified) flang/test/Lower/OpenMP/array-bounds.f90 (+4-4)
  • (added) flang/test/Lower/OpenMP/derived-type-map.f90 (+133)
  • (modified) flang/test/Lower/OpenMP/map-component-ref.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/target.f90 (+2-2)
  • (renamed) flang/test/Transforms/omp-map-info-finalization.fir (+29-5)
diff --git a/flang/docs/OpenMP-descriptor-management.md b/flang/docs/OpenMP-descriptor-management.md
index 90a20282e05126..af02b3a99cb07d 100644
--- a/flang/docs/OpenMP-descriptor-management.md
+++ b/flang/docs/OpenMP-descriptor-management.md
@@ -44,7 +44,7 @@ Currently, Flang will lower these descriptor types in the OpenMP lowering (lower
 to all other map types, generating an omp.MapInfoOp containing relevant information required for lowering
 the OpenMP dialect to LLVM-IR during the final stages of the MLIR lowering. However, after 
 the lowering to FIR/HLFIR has been performed an OpenMP dialect specific pass for Fortran, 
-`OMPDescriptorMapInfoGenPass` (Optimizer/OMPDescriptorMapInfoGen.cpp) will expand the 
+`OMPMapInfoFinalizationPass` (Optimizer/OMPMapInfoFinalization.cpp) will expand the 
 `omp.MapInfoOp`'s containing descriptors (which currently will be a `BoxType` or `BoxAddrOp`) into multiple 
 mappings, with one extra per pointer member in the descriptor that is supported on top of the original
 descriptor map operation. These pointers members are linked to the parent descriptor by adding them to 
@@ -52,7 +52,7 @@ the member field of the original descriptor map operation, they are then inserte
 owning operation's (`omp.TargetOp`, `omp.DataOp` etc.) map operand list and in cases where the owning operation
 is `IsolatedFromAbove`, it also inserts them as `BlockArgs` to canonicalize the mappings and simplify lowering.
 
-An example transformation by the `OMPDescriptorMapInfoGenPass`:
+An example transformation by the `OMPMapInfoFinalizationPass`:
 
 ```
 
diff --git a/flang/include/flang/Optimizer/Transforms/Passes.h b/flang/include/flang/Optimizer/Transforms/Passes.h
index e1d22c8c986da7..fc9a098c3931d3 100644
--- a/flang/include/flang/Optimizer/Transforms/Passes.h
+++ b/flang/include/flang/Optimizer/Transforms/Passes.h
@@ -76,7 +76,7 @@ std::unique_ptr<mlir::Pass>
 createAlgebraicSimplificationPass(const mlir::GreedyRewriteConfig &config);
 std::unique_ptr<mlir::Pass> createPolymorphicOpConversionPass();
 
-std::unique_ptr<mlir::Pass> createOMPDescriptorMapInfoGenPass();
+std::unique_ptr<mlir::Pass> createOMPMapInfoFinalizationPass();
 std::unique_ptr<mlir::Pass> createOMPFunctionFilteringPass();
 std::unique_ptr<mlir::OperationPass<mlir::ModuleOp>>
 createOMPMarkDeclareTargetPass();
diff --git a/flang/include/flang/Optimizer/Transforms/Passes.td b/flang/include/flang/Optimizer/Transforms/Passes.td
index 5fb576fd876254..0638ae49f5f4ea 100644
--- a/flang/include/flang/Optimizer/Transforms/Passes.td
+++ b/flang/include/flang/Optimizer/Transforms/Passes.td
@@ -318,15 +318,15 @@ def LoopVersioning : Pass<"loop-versioning", "mlir::func::FuncOp"> {
   let dependentDialects = [ "fir::FIROpsDialect" ];
 }
 
-def OMPDescriptorMapInfoGenPass
-    : Pass<"omp-descriptor-map-info-gen", "mlir::func::FuncOp"> {
+def OMPMapInfoFinalizationPass
+    : Pass<"omp-map-info-finalization", "mlir::func::FuncOp"> {
   let summary = "expands OpenMP MapInfo operations containing descriptors";
   let description = [{
     Expands MapInfo operations containing descriptor types into multiple 
     MapInfo's for each pointer element in the descriptor that requires 
     explicit individual mapping by the OpenMP runtime.
   }];
-  let constructor = "::fir::createOMPDescriptorMapInfoGenPass()";
+  let constructor = "::fir::createOMPMapInfoFinalizationPass()";
   let dependentDialects = ["mlir::omp::OpenMPDialect"];
 }
 
diff --git a/flang/include/flang/Tools/CLOptions.inc b/flang/include/flang/Tools/CLOptions.inc
index 68e504d0ccb512..ec3d634ac0264b 100644
--- a/flang/include/flang/Tools/CLOptions.inc
+++ b/flang/include/flang/Tools/CLOptions.inc
@@ -274,7 +274,7 @@ inline void createHLFIRToFIRPassPipeline(
 /// rather than the host device.
 inline void createOpenMPFIRPassPipeline(
     mlir::PassManager &pm, bool isTargetDevice) {
-  pm.addPass(fir::createOMPDescriptorMapInfoGenPass());
+  pm.addPass(fir::createOMPMapInfoFinalizationPass());
   pm.addPass(fir::createOMPMarkDeclareTargetPass());
   if (isTargetDevice)
     pm.addPass(fir::createOMPFunctionFilteringPass());
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index a41f8312a28c9e..6045d6daf8d41e 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -843,9 +843,10 @@ mlir::omp::MapInfoOp
 createMapInfoOp(fir::FirOpBuilder &builder, mlir::Location loc,
                 mlir::Value baseAddr, mlir::Value varPtrPtr, std::string name,
                 mlir::SmallVector<mlir::Value> bounds,
-                mlir::SmallVector<mlir::Value> members, uint64_t mapType,
+                mlir::SmallVector<mlir::Value> members,
+                mlir::ArrayAttr membersIndex, uint64_t mapType,
                 mlir::omp::VariableCaptureKind mapCaptureType, mlir::Type retTy,
-                bool isVal) {
+                bool partialMap) {
   if (auto boxTy = baseAddr.getType().dyn_cast<fir::BaseBoxType>()) {
     baseAddr = builder.create<fir::BoxAddrOp>(loc, baseAddr);
     retTy = baseAddr.getType();
@@ -855,10 +856,10 @@ createMapInfoOp(fir::FirOpBuilder &builder, mlir::Location loc,
       llvm::cast<mlir::omp::PointerLikeType>(retTy).getElementType());
 
   mlir::omp::MapInfoOp op = builder.create<mlir::omp::MapInfoOp>(
-      loc, retTy, baseAddr, varType, varPtrPtr, members, bounds,
+      loc, retTy, baseAddr, varType, varPtrPtr, members, membersIndex, bounds,
       builder.getIntegerAttr(builder.getIntegerType(64, false), mapType),
       builder.getAttr<mlir::omp::VariableCaptureKindAttr>(mapCaptureType),
-      builder.getStringAttr(name));
+      builder.getStringAttr(name), builder.getBoolAttr(partialMap));
 
   return op;
 }
@@ -867,12 +868,16 @@ bool ClauseProcessor::processMap(
     mlir::Location currentLocation, const llvm::omp::Directive &directive,
     Fortran::lower::StatementContext &stmtCtx,
     llvm::SmallVectorImpl<mlir::Value> &mapOperands,
+    llvm::SmallVectorImpl<const Fortran::semantics::Symbol *> *mapSymbols,
     llvm::SmallVectorImpl<mlir::Type> *mapSymTypes,
-    llvm::SmallVectorImpl<mlir::Location> *mapSymLocs,
-    llvm::SmallVectorImpl<const Fortran::semantics::Symbol *> *mapSymbols)
-    const {
+    llvm::SmallVectorImpl<mlir::Location> *mapSymLocs) const {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
-  return findRepeatableClause<ClauseTy::Map>(
+
+  llvm::SmallVector<mlir::omp::MapInfoOp> memberMaps;
+  llvm::SmallVector<mlir::Attribute> memberPlacementIndices;
+  llvm::SmallVector<const Fortran::semantics::Symbol *> memberParentSyms;
+
+  bool clauseFound = findRepeatableClause<ClauseTy::Map>(
       [&](const ClauseTy::Map *mapClause,
           const Fortran::parser::CharBlock &source) {
         mlir::Location clauseLocation = converter.genLocation(source);
@@ -919,8 +924,27 @@ bool ClauseProcessor::processMap(
 
         for (const Fortran::parser::OmpObject &ompObject :
              std::get<Fortran::parser::OmpObjectList>(mapClause->v.t).v) {
+          llvm::omp::OpenMPOffloadMappingFlags objectsMapTypeBits = mapTypeBits;
+          checkAndApplyDeclTargetMapFlags(converter, objectsMapTypeBits,
+                                          *getOmpObjectSymbol(ompObject));
+
           llvm::SmallVector<mlir::Value> bounds;
           std::stringstream asFortran;
+          const Fortran::semantics::Symbol *parentSym = nullptr;
+
+          if (getOmpObjectSymbol(ompObject)->owner().IsDerivedType()) {
+            const auto *designator =
+                Fortran::parser::Unwrap<Fortran::parser::Designator>(
+                    ompObject.u);
+            assert(designator && "Expected a designator from derived type "
+                                 "component during map clause processing");
+            parentSym = GetFirstName(*designator).symbol;
+            memberParentSyms.push_back(parentSym);
+            memberPlacementIndices.push_back(
+                firOpBuilder.getI64IntegerAttr(findComponentMemberPlacement(
+                    &parentSym->GetType()->derivedTypeSpec().typeSymbol(),
+                    getOmpObjectSymbol(ompObject))));
+          }
 
           Fortran::lower::AddrAndBoundsInfo info =
               Fortran::lower::gatherDataOperandAddrAndBounds<
@@ -938,24 +962,32 @@ bool ClauseProcessor::processMap(
           // Explicit map captures are captured ByRef by default,
           // optimisation passes may alter this to ByCopy or other capture
           // types to optimise
-          mlir::Value mapOp = createMapInfoOp(
+          mlir::omp::MapInfoOp mapOp = createMapInfoOp(
               firOpBuilder, clauseLocation, symAddr, mlir::Value{},
-              asFortran.str(), bounds, {},
+              asFortran.str(), bounds, {}, mlir::ArrayAttr{},
               static_cast<
                   std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>(
-                  mapTypeBits),
+                  objectsMapTypeBits),
               mlir::omp::VariableCaptureKind::ByRef, symAddr.getType());
 
-          mapOperands.push_back(mapOp);
-          if (mapSymTypes)
-            mapSymTypes->push_back(symAddr.getType());
-          if (mapSymLocs)
-            mapSymLocs->push_back(symAddr.getLoc());
-
-          if (mapSymbols)
+          if (parentSym) {
+            memberMaps.push_back(mapOp);
+          } else {
+            mapOperands.push_back(mapOp);
             mapSymbols->push_back(getOmpObjectSymbol(ompObject));
+            if (mapSymTypes)
+              mapSymTypes->push_back(symAddr.getType());
+            if (mapSymLocs)
+              mapSymLocs->push_back(symAddr.getLoc());
+          }
         }
       });
+
+  insertChildMapInfoIntoParent(converter, memberParentSyms, memberMaps,
+                               memberPlacementIndices, mapOperands, mapSymTypes,
+                               mapSymLocs, mapSymbols);
+
+  return clauseFound;
 }
 
 bool ClauseProcessor::processReduction(
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.h b/flang/lib/Lower/OpenMP/ClauseProcessor.h
index 11aff0be25053d..5f97ebf63e93c1 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.h
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.h
@@ -114,14 +114,13 @@ class ClauseProcessor {
   // store the original type, location and Fortran symbol for the map operands.
   // They may be used later on to create the block_arguments for some of the
   // target directives that require it.
-  bool processMap(mlir::Location currentLocation,
-                  const llvm::omp::Directive &directive,
-                  Fortran::lower::StatementContext &stmtCtx,
-                  llvm::SmallVectorImpl<mlir::Value> &mapOperands,
-                  llvm::SmallVectorImpl<mlir::Type> *mapSymTypes = nullptr,
-                  llvm::SmallVectorImpl<mlir::Location> *mapSymLocs = nullptr,
-                  llvm::SmallVectorImpl<const Fortran::semantics::Symbol *>
-                      *mapSymbols = nullptr) const;
+  bool processMap(
+      mlir::Location currentLocation, const llvm::omp::Directive &directive,
+      Fortran::lower::StatementContext &stmtCtx,
+      llvm::SmallVectorImpl<mlir::Value> &mapOperands,
+      llvm::SmallVectorImpl<const Fortran::semantics::Symbol *> *mapSymbols,
+      llvm::SmallVectorImpl<mlir::Type> *mapSymTypes = nullptr,
+      llvm::SmallVectorImpl<mlir::Location> *mapSymLocs = nullptr) const;
   bool
   processReduction(mlir::Location currentLocation,
                    llvm::SmallVectorImpl<mlir::Value> &reductionVars,
@@ -188,7 +187,12 @@ template <typename T>
 bool ClauseProcessor::processMotionClauses(
     Fortran::lower::StatementContext &stmtCtx,
     llvm::SmallVectorImpl<mlir::Value> &mapOperands) {
-  return findRepeatableClause<T>(
+  llvm::SmallVector<mlir::omp::MapInfoOp> memberMaps;
+  llvm::SmallVector<mlir::Attribute> memberPlacementIndices;
+  llvm::SmallVector<const Fortran::semantics::Symbol *> memberParentSyms,
+      mapSymbols;
+
+  bool clauseFound = findRepeatableClause<T>(
       [&](const T *motionClause, const Fortran::parser::CharBlock &source) {
         mlir::Location clauseLocation = converter.genLocation(source);
         fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
@@ -203,8 +207,28 @@ bool ClauseProcessor::processMotionClauses(
                 : llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
 
         for (const Fortran::parser::OmpObject &ompObject : motionClause->v.v) {
+          llvm::omp::OpenMPOffloadMappingFlags objectsMapTypeBits = mapTypeBits;
+          checkAndApplyDeclTargetMapFlags(converter, objectsMapTypeBits,
+                                          *getOmpObjectSymbol(ompObject));
+
           llvm::SmallVector<mlir::Value> bounds;
           std::stringstream asFortran;
+          const Fortran::semantics::Symbol *parentSym = nullptr;
+
+          if (getOmpObjectSymbol(ompObject)->owner().IsDerivedType()) {
+            const auto *designator =
+                Fortran::parser::Unwrap<Fortran::parser::Designator>(
+                    ompObject.u);
+            assert(designator && "Expected a designator from derived type "
+                                 "component during motion clause processing");
+            parentSym = GetFirstName(*designator).symbol;
+            memberParentSyms.push_back(parentSym);
+            memberPlacementIndices.push_back(
+                firOpBuilder.getI64IntegerAttr(findComponentMemberPlacement(
+                    &parentSym->GetType()->derivedTypeSpec().typeSymbol(),
+                    getOmpObjectSymbol(ompObject))));
+          }
+
           Fortran::lower::AddrAndBoundsInfo info =
               Fortran::lower::gatherDataOperandAddrAndBounds<
                   Fortran::parser::OmpObject, mlir::omp::DataBoundsOp,
@@ -221,17 +245,27 @@ bool ClauseProcessor::processMotionClauses(
           // Explicit map captures are captured ByRef by default,
           // optimisation passes may alter this to ByCopy or other capture
           // types to optimise
-          mlir::Value mapOp = createMapInfoOp(
+          mlir::omp::MapInfoOp mapOp = createMapInfoOp(
               firOpBuilder, clauseLocation, symAddr, mlir::Value{},
-              asFortran.str(), bounds, {},
+              asFortran.str(), bounds, {}, mlir::ArrayAttr{},
               static_cast<
                   std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>(
-                  mapTypeBits),
+                  objectsMapTypeBits),
               mlir::omp::VariableCaptureKind::ByRef, symAddr.getType());
 
-          mapOperands.push_back(mapOp);
+          if (parentSym) {
+            memberMaps.push_back(mapOp);
+          } else {
+            mapOperands.push_back(mapOp);
+            mapSymbols.push_back(getOmpObjectSymbol(ompObject));
+          }
         }
       });
+
+  insertChildMapInfoIntoParent(converter, memberParentSyms, memberMaps,
+                               memberPlacementIndices, mapOperands, nullptr,
+                               nullptr, &mapSymbols);
+  return clauseFound;
 }
 
 template <typename... Ts>
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 7953bf83cba0fe..da497fe139c78a 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -735,7 +735,8 @@ genDataOp(Fortran::lower::AbstractConverter &converter,
       deviceAddrOperands;
   llvm::SmallVector<mlir::Type> useDeviceTypes;
   llvm::SmallVector<mlir::Location> useDeviceLocs;
-  llvm::SmallVector<const Fortran::semantics::Symbol *> useDeviceSymbols;
+  llvm::SmallVector<const Fortran::semantics::Symbol *> useDeviceSymbols,
+      mapSymbols;
 
   ClauseProcessor cp(converter, semaCtx, clauseList);
   cp.processIf(Fortran::parser::OmpIfClause::DirectiveNameModifier::TargetData,
@@ -746,7 +747,7 @@ genDataOp(Fortran::lower::AbstractConverter &converter,
   cp.processUseDeviceAddr(deviceAddrOperands, useDeviceTypes, useDeviceLocs,
                           useDeviceSymbols);
   cp.processMap(currentLocation, llvm::omp::Directive::OMPD_target_data,
-                stmtCtx, mapOperands);
+                stmtCtx, mapOperands, &mapSymbols);
 
   auto dataOp = converter.getFirOpBuilder().create<mlir::omp::DataOp>(
       currentLocation, ifClauseOperand, deviceOperand, devicePtrOperands,
@@ -769,6 +770,7 @@ genEnterExitUpdateDataOp(Fortran::lower::AbstractConverter &converter,
   mlir::UnitAttr nowaitAttr;
   llvm::SmallVector<mlir::Value> mapOperands, dependOperands;
   llvm::SmallVector<mlir::Attribute> dependTypeOperands;
+  llvm::SmallVector<const Fortran::semantics::Symbol *> mapSymbols;
 
   Fortran::parser::OmpIfClause::DirectiveNameModifier directiveName;
   llvm::omp::Directive directive;
@@ -801,7 +803,8 @@ genEnterExitUpdateDataOp(Fortran::lower::AbstractConverter &converter,
                                                               mapOperands);
 
   } else {
-    cp.processMap(currentLocation, directive, stmtCtx, mapOperands);
+    cp.processMap(currentLocation, directive, stmtCtx, mapOperands,
+                  &mapSymbols);
   }
 
   return firOpBuilder.create<OpTy>(
@@ -922,7 +925,7 @@ static void genBodyOfTargetOp(
         firOpBuilder.setInsertionPoint(targetOp);
         mlir::Value mapOp = createMapInfoOp(
             firOpBuilder, copyVal.getLoc(), copyVal, mlir::Value{}, name.str(),
-            bounds, llvm::SmallVector<mlir::Value>{},
+            bounds, llvm::SmallVector<mlir::Value>{}, mlir::ArrayAttr{},
             static_cast<
                 std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>(
                 llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_IMPLICIT),
@@ -990,9 +993,8 @@ genTargetOp(Fortran::lower::AbstractConverter &converter,
   cp.processThreadLimit(stmtCtx, threadLimitOperand);
   cp.processDepend(dependTypeOperands, dependOperands);
   cp.processNowait(nowaitAttr);
-  cp.processMap(currentLocation, directive, stmtCtx, mapOperands, &mapSymTypes,
-                &mapSymLocs, &mapSymbols);
-
+  cp.processMap(currentLocation, directive, stmtCtx, mapOperands, &mapSymbols,
+                &mapSymTypes, &mapSymLocs);
   cp.processTODO<Fortran::parser::OmpClause::Private,
                  Fortran::parser::OmpClause::Firstprivate,
                  Fortran::parser::OmpClause::IsDevicePtr,
@@ -1057,9 +1059,11 @@ genTargetOp(Fortran::lower::AbstractConverter &converter,
           mapFlag |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
         }
 
+        checkAndApplyDeclTargetMapFlags(converter, mapFlag, sym);
+
         mlir::Value mapOp = createMapInfoOp(
             converter.getFirOpBuilder(), baseOp.getLoc(), baseOp, mlir::Value{},
-            name.str(), bounds, {},
+            name.str(), bounds, {}, mlir::ArrayAttr{},
             static_cast<
                 std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>(
                 mapFlag),
diff --git a/flang/lib/Lower/OpenMP/Utils.cpp b/flang/lib/Lower/OpenMP/Utils.cpp
index 31b15257d18687..fc8e5b2c72011d 100644
--- a/flang/lib/Lower/OpenMP/Utils.cpp
+++ b/flang/lib/Lower/OpenMP/Utils.cpp
@@ -14,6 +14,7 @@
 
 #include <flang/Lower/AbstractConverter.h>
 #include <flang/Lower/ConvertType.h>
+#include <flang/Optimizer/Builder/FIRBuilder.h>
 #include <flang/Parser/parse-tree.h>
 #include <flang/Parser/tools.h>
 #include <flang/Semantics/tools.h>
@@ -70,6 +71,105 @@ void gatherFuncAndVarSyms(
   }
 }
 
+void checkAndApplyDeclTargetMapFlags(
+    Fortran::lower::AbstractConverter &converter,
+    llvm::omp::OpenMPOffloadMappingFlags &mapFlags,
+    const Fortran::semantics::Symbol &symbol) {
+  if (auto declareTargetOp =
+          llvm::dyn_cast_if_present<mlir::omp::DeclareTargetInterface>(
+              converter.getModuleOp().lookupSymbol(
+                  converter.mangleName(symbol)))) {
+    // Only Link clauses have OMP_MAP_PTR_AND_OBJ applied, To clause
+    // seems to function differently.
+    if (declareTargetOp.getDeclareTargetCaptureClause() ==
+        mlir::omp::DeclareTargetCaptureClause::link)
+      mapFlags |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_PTR_AND_OBJ;
+  }
+}
+
+int findComponentMemberPlacement(
+    const Fortran::semantics::Symbol *dTypeSym,
+    const Fortran::semantics::Symbol *componentSym) {
+  const auto *derived =
+      dTypeSym->detailsIf<Fortran::semantics::DerivedTypeDetails>();
+  i...
[truncated]

@clementval
Copy link
Contributor

You could do the renaming of the pass as an NFC PR so it would make the non NFC changes more obvious to spot at. Not necessary to do it for this time but next time it would be nice.

@agozillon
Copy link
Contributor Author

You could do the renaming of the pass as an NFC PR so it would make the non NFC changes more obvious to spot at. Not necessary to do it for this time but next time it would be nice.

I'm very sorry about that! I didn't realise it would be a problem, but I will keep it in mind for the future for any renaming + change sets I aim to upstream. Thank you for bringing it up.

@clementval
Copy link
Contributor

You could do the renaming of the pass as an NFC PR so it would make the non NFC changes more obvious to spot at. Not necessary to do it for this time but next time it would be nice.

I'm very sorry about that! I didn't realise it would be a problem, but I will keep it in mind for the future for any renaming + change sets I aim to upstream. Thank you for bringing it up.

Not an issue at all. It's just simpler to look at small PRs :-)

@agozillon
Copy link
Contributor Author

agozillon commented Feb 24, 2024

I have opened PRs again on the entire stack, hopefully they shouldn't cause problems for windows users anymore, I've checked the length of the largest PR (and intermediate commits SPR seems to make) and I believe it fits within the maximum that we're planning on enforcing.

@agozillon
Copy link
Contributor Author

You could do the renaming of the pass as an NFC PR so it would make the non NFC changes more obvious to spot at. Not necessary to do it for this time but next time it would be nice.

I'm very sorry about that! I didn't realise it would be a problem, but I will keep it in mind for the future for any renaming + change sets I aim to upstream. Thank you for bringing it up.

Not an issue at all. It's just simpler to look at small PRs :-)

Yes, I am sorry about that, I'd like to make it as easy as possible to review for you all as I know it's not an easy task and I greatly appreciate the effort you all put in, so I will most definitely keep it mind in the future!

@clementval
Copy link
Contributor

Yes, I am sorry about that, I'd like to make it as easy as possible to review for you all as I know it's not an easy task and I greatly appreciate the effort you all put in, so I will most definitely keep it mind in the future!

Thanks!

@agozillon
Copy link
Contributor Author

Small ping for attention on this PR please, to start nudging it towards the finish line if there is one in its future. I am sorry ahead of time for stealing some of a reviewers precious time.

ompObject.u);
assert(designator && "Expected a designator from derived type "
"component during map clause processing");
parentSym = GetFirstName(*designator).symbol;
Copy link
Contributor

Choose a reason for hiding this comment

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

We will need to traverse all parents. For example given x%y%z, GetFirstName will return x. The type of x does not have a member z, so findComponentMemberPlacement will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR isn't intended to handle that type of mapping yet, it'll be handled in a subsequent series (which I am working on at the moment).

It's only intended to handle cases where we're mapping one level of nesting for the moment.

agozillon added a commit to agozillon/llvm-project that referenced this pull request Mar 13, 2024
This patch is one in a series of four patches that seeks to refactor
slightly and extend the current record type map support that was
put in place for Fortran's descriptor types to handle explicit
member mapping for record types at a single level of depth.

For example, the below case where two members of a Fortran
derived type are mapped explicitly:

''''
  type :: scalar_and_array
    real(4) :: real
    integer(4) :: array(10)
    integer(4) :: int
  end type scalar_and_array
  type(scalar_and_array) :: scalar_arr

  !$omp target map(tofrom: scalar_arr%int, scalar_arr%real)
''''

Current cases of derived type mapping left for future work are:
  > explicit member mapping of nested members (e.g. two layers of
     record types where we explicitly map a member from the internal
     record type)
  > Fortran's automagical mapping of all elements and nested elements
     of a derived type
  > explicit member mapping of a derived type and then constituient members
     (redundant in Fortran due to former case but still legal as far as I am aware)
  > explicit member mapping of a record type (may be handled reasonably, just
     not fully tested in this iteration)
  > explicit member mapping for Fortran allocatable types (a variation of nested
     record types)

This patch seeks to support this by extending the Flang-new OpenMP lowering to
support generation of this newly required information, creating the neccessary
parent <-to-> member map_info links, calculating the member indices and
setting if it's a partial map.

The OMPDescriptorMapInfoGen pass has also been generalized into a map
finalization phase, now named OMPMapInfoFinalization. This pass was extended
to support the insertion of member maps into the BlockArg and MapOperands of
relevant map carrying operations. Similar to the method in which descriptor types
are expanded and constituient members inserted.

Pull Request: llvm#82853
agozillon added a commit to agozillon/llvm-project that referenced this pull request Mar 15, 2024
This patch is one in a series of four patches that seeks to refactor
slightly and extend the current record type map support that was
put in place for Fortran's descriptor types to handle explicit
member mapping for record types at a single level of depth.

For example, the below case where two members of a Fortran
derived type are mapped explicitly:

''''
  type :: scalar_and_array
    real(4) :: real
    integer(4) :: array(10)
    integer(4) :: int
  end type scalar_and_array
  type(scalar_and_array) :: scalar_arr

  !$omp target map(tofrom: scalar_arr%int, scalar_arr%real)
''''

Current cases of derived type mapping left for future work are:
  > explicit member mapping of nested members (e.g. two layers of
     record types where we explicitly map a member from the internal
     record type)
  > Fortran's automagical mapping of all elements and nested elements
     of a derived type
  > explicit member mapping of a derived type and then constituient members
     (redundant in Fortran due to former case but still legal as far as I am aware)
  > explicit member mapping of a record type (may be handled reasonably, just
     not fully tested in this iteration)
  > explicit member mapping for Fortran allocatable types (a variation of nested
     record types)

This patch seeks to support this by extending the Flang-new OpenMP lowering to
support generation of this newly required information, creating the neccessary
parent <-to-> member map_info links, calculating the member indices and
setting if it's a partial map.

The OMPDescriptorMapInfoGen pass has also been generalized into a map
finalization phase, now named OMPMapInfoFinalization. This pass was extended
to support the insertion of member maps into the BlockArg and MapOperands of
relevant map carrying operations. Similar to the method in which descriptor types
are expanded and constituient members inserted.

Pull Request: llvm#82853
@agozillon
Copy link
Contributor Author

The current PR stack has been updated with the aims to extend support to explicit derived type member mapping at arbitrary depths, and not just top level member mapping. The change set is slightly different, primarily in the sense that we now need to handle index generation in Flang and then first/last member selection in the lowering differently due to the new attribute type in use for member placement tracking (DenseIntElementsAttr, for N-D indices).

These are the encompassing PR stack that were updated:

#82853
#82852
#82851
#82850

The only PR from the stack that is still a little in flux so I would recommend not reviewing for a little while (I'll add a comment when it's ready), is the MLIR -> LLVM-IR lowering (#82852), as it has a partially duplicate change set that's segmented into another PR (#84349) for hopefully easier/quicker review. Once the other has landed I hope to perform a rebase to align the stack completely. That being said, if you do review it, I'll do my best to apply the requests to the relevant PR.

agozillon added a commit to agozillon/llvm-project that referenced this pull request Mar 20, 2024
This patch is one in a series of four patches that seeks to refactor
slightly and extend the current record type map support that was
put in place for Fortran's descriptor types to handle explicit
member mapping for record types at a single level of depth.

For example, the below case where two members of a Fortran
derived type are mapped explicitly:

''''
  type :: scalar_and_array
    real(4) :: real
    integer(4) :: array(10)
    integer(4) :: int
  end type scalar_and_array
  type(scalar_and_array) :: scalar_arr

  !$omp target map(tofrom: scalar_arr%int, scalar_arr%real)
''''

Current cases of derived type mapping left for future work are:
  > explicit member mapping of nested members (e.g. two layers of
     record types where we explicitly map a member from the internal
     record type)
  > Fortran's automagical mapping of all elements and nested elements
     of a derived type
  > explicit member mapping of a derived type and then constituient members
     (redundant in Fortran due to former case but still legal as far as I am aware)
  > explicit member mapping of a record type (may be handled reasonably, just
     not fully tested in this iteration)
  > explicit member mapping for Fortran allocatable types (a variation of nested
     record types)

This patch seeks to support this by extending the Flang-new OpenMP lowering to
support generation of this newly required information, creating the neccessary
parent <-to-> member map_info links, calculating the member indices and
setting if it's a partial map.

The OMPDescriptorMapInfoGen pass has also been generalized into a map
finalization phase, now named OMPMapInfoFinalization. This pass was extended
to support the insertion of member maps into the BlockArg and MapOperands of
relevant map carrying operations. Similar to the method in which descriptor types
are expanded and constituient members inserted.

Pull Request: llvm#82853
Created using spr 1.3.4
@agozillon
Copy link
Contributor Author

Rebased the entire stack on recent upstream changes, there was quite a few changes that meant some sizeable alterations to the Fortran lowering segment of the PR stack, but otherwise it all went smoothly. However, please do point out if you see anything out of the ordinary across the change set that may look like a rebase artifact when reviewing!

Otherwise, this PR stack is now fully ready for review so please do so if you have some time, it would be greatly appreciated.

@agozillon
Copy link
Contributor Author

Small ping for review of this PR stack if possible. I'll be away on vacation for two and a half weeks, but it would be excellent to come back to review comments on this PR stack that I can address and then make some forward progress on. No rush obviously as there will be a 2 week time lag before it's all addressed but reviews would be appreciated!

@agozillon
Copy link
Contributor Author

Back from vacation, would very much love if a kind reviewer or two could please allocate some time to review this PR stack so I can hopefully move it further along in the next few weeks :-) Thank you ahead of time!

Created using spr 1.3.4
Copy link
Member

@skatrak skatrak left a comment

Choose a reason for hiding this comment

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

Thank you for all the work here Andrew, I've got some comments but they should be relatively easy to address.

@agozillon
Copy link
Contributor Author

Performed a rebase across the entire PR stack (my appologies, I forgot that might make the review a little more irritating before I performed it), and made an attempt to address all reviewer comments and where things didn't quite work as expected left replies on the comments.

agozillon added a commit to agozillon/llvm-project that referenced this pull request Apr 26, 2024
This patch is one in a series of four patches that seeks to refactor
slightly and extend the current record type map support that was
put in place for Fortran's descriptor types to handle explicit
member mapping for record types at a single level of depth.

For example, the below case where two members of a Fortran
derived type are mapped explicitly:

''''
  type :: scalar_and_array
    real(4) :: real
    integer(4) :: array(10)
    integer(4) :: int
  end type scalar_and_array
  type(scalar_and_array) :: scalar_arr

  !$omp target map(tofrom: scalar_arr%int, scalar_arr%real)
''''

Current cases of derived type mapping left for future work are:
  > explicit member mapping of nested members (e.g. two layers of
     record types where we explicitly map a member from the internal
     record type)
  > Fortran's automagical mapping of all elements and nested elements
     of a derived type
  > explicit member mapping of a derived type and then constituient members
     (redundant in Fortran due to former case but still legal as far as I am aware)
  > explicit member mapping of a record type (may be handled reasonably, just
     not fully tested in this iteration)
  > explicit member mapping for Fortran allocatable types (a variation of nested
     record types)

This patch seeks to support this by extending the Flang-new OpenMP lowering to
support generation of this newly required information, creating the neccessary
parent <-to-> member map_info links, calculating the member indices and
setting if it's a partial map.

The OMPDescriptorMapInfoGen pass has also been generalized into a map
finalization phase, now named OMPMapInfoFinalization. This pass was extended
to support the insertion of member maps into the BlockArg and MapOperands of
relevant map carrying operations. Similar to the method in which descriptor types
are expanded and constituient members inserted.

Pull Request: llvm#82853
Copy link
Contributor

@jsjodin jsjodin left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor nits/comments.

/// The pass also adds MapInfoOp's that are members of a parent object but are
/// not directly used in the body of a target region to its BlockArgument list
/// to maintain consistency across all MapInfoOp's tied to a region directly or
/// indirectly via an parent object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: an -> a

//
// The parent member becomes more critical, as we perform a partial
// structure mapping where we link the mapping of x and y together
// via the parent. We do this at a kernel argument level in LLVM IR
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Maybe include how 'z' is handled.

// as the printing and later processing currently requires a 1:1
// mapping of BlockArgs to MapInfoOp's at the same placement in
// each array (BlockArgs and MapOperands).
if (auto targetOp = llvm::dyn_cast<mlir::omp::TargetOp>(target)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The dyn_cast could be moved outside the loops.

static_cast<
std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>(
mapTypeBits),
mlir::omp::VariableCaptureKind::ByRef, symAddr.getType());

result.mapVars.push_back(mapOp);
if (object.id()->owner().IsDerivedType()) {
std::optional<Fortran::evaluate::DataRef> dataRef =
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to create a function for the code inside the 'if'? It seems like the same code exists above.

uint64_t mapType = indices.second[0].memberMap.getMapType().value_or(0);

// create parent to emplace and bind members
auto origSymbol = converter.getSymbolAddress(*indices.first);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think explicit types should be used if the type isn't specified on the rhs.

auto origSymbol = converter.getSymbolAddress(*indices.first);

llvm::SmallVector<mlir::Value> members;
for (auto memberIndicesData : indices.second)
Copy link
Contributor

Choose a reason for hiding this comment

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

Explicit type here also.

@agozillon
Copy link
Contributor Author

Thanks for the review @jsjodin and @skatrak I've attempted to address both of your comments in recent updates.

@jsjodin jsjodin self-requested a review May 2, 2024 14:23
Copy link
Contributor

@jsjodin jsjodin left a comment

Choose a reason for hiding this comment

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

LGTM

Created using spr 1.3.4
Copy link
Member

@skatrak skatrak left a comment

Choose a reason for hiding this comment

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

Thank you Andrew, you've addressed all my concerns so this LGTM.

shape.push_back(largestIndicesSize);

// DenseElementsAttr expects a rectangular shape for the data, so all
// index lists have to be of the same length, this emplaces -1 as filler
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// index lists have to be of the same length, this emplaces -1 as filler
// index lists have to be of the same length, this emplaces -1 as filler.

@agozillon
Copy link
Contributor Author

Thank you all very much for the reviews across the PR stack!

I believe I have enough acceptance for the PR stack to land now, with at least two reviewers per PR. So I will seek to land this PR stack in the next few days (on Thursday) I'll perform the final adjustments requested, rebase and test prior to landing.

However, if anyone has any concerns or further review comments for any segments, please mention them now and I can hold off the PR until they are addressed if necessary :-)

Created using spr 1.3.4
@agozillon agozillon merged commit e8fabf9 into users/agozillon/main.flangopenmpmlir-initial-derived-type-member-map-support May 10, 2024
6 of 7 checks passed
@agozillon agozillon deleted the users/agozillon/flangopenmpmlir-initial-derived-type-member-map-support branch May 10, 2024 18:07
@agozillon agozillon restored the users/agozillon/flangopenmpmlir-initial-derived-type-member-map-support branch May 10, 2024 18:13
@agozillon agozillon deleted the users/agozillon/flangopenmpmlir-initial-derived-type-member-map-support branch May 10, 2024 18:13
agozillon added a commit that referenced this pull request May 10, 2024
This patch is one in a series of four patches that seeks to refactor
slightly and extend the current record type map support that was
put in place for Fortran's descriptor types to handle explicit
member mapping for record types at a single level of depth.

For example, the below case where two members of a Fortran
derived type are mapped explicitly:

''''
  type :: scalar_and_array
    real(4) :: real
    integer(4) :: array(10)
    integer(4) :: int
  end type scalar_and_array
  type(scalar_and_array) :: scalar_arr

  !$omp target map(tofrom: scalar_arr%int, scalar_arr%real)
''''

Current cases of derived type mapping left for future work are:
  > explicit member mapping of nested members (e.g. two layers of
     record types where we explicitly map a member from the internal
     record type)
  > Fortran's automagical mapping of all elements and nested elements
     of a derived type
  > explicit member mapping of a derived type and then constituient members
     (redundant in Fortran due to former case but still legal as far as I am aware)
  > explicit member mapping of a record type (may be handled reasonably, just
     not fully tested in this iteration)
  > explicit member mapping for Fortran allocatable types (a variation of nested
     record types)

This patch seeks to support this by extending the Flang-new OpenMP lowering to
support generation of this newly required information, creating the neccessary
parent <-to-> member map_info links, calculating the member indices and
setting if it's a partial map.

The OMPDescriptorMapInfoGen pass has also been generalized into a map
finalization phase, now named OMPMapInfoFinalization. This pass was extended
to support the insertion of member maps into the BlockArg and MapOperands of
relevant map carrying operations. Similar to the method in which descriptor types
are expanded and constituient members inserted.

Pull Request: #82853
@agozillon
Copy link
Contributor Author

I made a bit of a mistake not using SPR to land this PR series, so i've ended up having to directly commit the PR, the associated commit for this PR can be found here: 435e850

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.

6 participants