[MLIR][OpenMP] Add Lowering support for OpenMP Target Data, Exit Data and Enter Data directives

[TIFitis]

Summary

This patch adds Fortran Lowering support for the OpenMP Target Data, Target Exit Data and Target Enter Data constructs.
operation.

[clementval]

flang/lib/Lower/OpenMP.cpp

568

Same comment as below for the currentLocation.

571–574

In the lowering we use fully qualified name for stuff from mlir and llvm. Can you update the code here and in other places?

685

Spell out auto here.

685

Becareful, the current location is probably set to the last Fortran construct and not on the openmp directive itself. Look at https://reviews.llvm.org/D131659 to see how we did it for OpenACC.

[TIFitis]

Small fixes. Fixed linting.

[TIFitis]

Added tests. Addressed reviewer comments.

[TIFitis]

flang/lib/Lower/OpenMP.cpp

685

I've tried to fix this issue in both places. Please let me know if it's still wrong.

[kiranchandramohan]

Looks mostly fine. Please see comments inline.

flang/lib/Lower/OpenMP.cpp

599

Is that release for exit and alloc for enter/data?
Could you give a pointer to the code?

607–609

Nit: Braces are not required.

610

Any specific reason for not including close and present?

658

Nit: Add an assert or a TODO unhandled for the default else.

679

Nit: Add an assert or a TODO unhandled for the default else.

flang/test/Lower/target_data.f90

106 ↗

(On Diff #491418)

Nit: No newline.

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp

152

There are no tests for the contents of this file. Move this to a separate patch.

[clementval]

flang/lib/Lower/OpenMP.cpp

685

You need to create a new location from the source of the clause or the directive. Something like:

mlir::Location clauseLocation = converter.genLocation(clause.source);

[TIFitis]

Addressed reviewer comments.

[TIFitis]

flang/lib/Lower/OpenMP.cpp

599

Sorry, what do you mean by give a pointer to the code?

I've expanded the comment to mention alloc is for target data and enter data, and release is for exit data.

610

Parser support is not present.

flang/test/Lower/target_data.f90

106 ↗

(On Diff #491418)

If you see the raw file there is no newline at the end of file.

I don't know why the "No newline at end of file" is shown here.

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp

152

I've remove these changes.

It enabled conversion from FIR Dialect to LLVM Dialect.

Can you please tell me where to add tests for this?

[kiranchandramohan]

flang/lib/Lower/OpenMP.cpp

599

Sorry, I meant a pointer to the runtime code.

Ideally, we should not assume anything about the OpenMP runtime. We are lowering to the OpenMP dialect here. Any such assumptions should be made clear in the dialect. Do we say something like this in the definition of the MLIR operation? Alternatively, we can pass the Alloc or Release and then during lowering from the OpenMP dialect to LLVM IR, this can be handled appropriately.

flang/test/Lower/target_data.f90

106 ↗

(On Diff #491418)

https://unix.stackexchange.com/questions/31947/how-to-add-a-newline-to-the-end-of-a-file

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp

152

flang side : flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
mlir side : mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir

[TIFitis]

Added new line to end of test file.

flang/lib/Lower/OpenMP.cpp

599

I've copied the comment from ClangCodeGen. OpenMPOffloadMappingFlags does not have enums for alloc and release and thus I can't really add them here.

Also, this is handled in the custom printer and parser, i.e, it still shows alloc or release appropriately in the fir and mlir. An absence of map_type implicitly assumes alloc or release based on the target directive.

ClangCodeGen reference: clang/lib/CodeGen/CGOpenMPRuntime.cpp::7065

flang/test/Lower/target_data.f90

106 ↗

(On Diff #491418)

Sorry, I thought you were asking me to remove the line instead of adding one.

[kiranchandramohan]

LGTM.

flang/lib/Lower/OpenMP.cpp

599

Nit: OK. Can you change the comment to refer to the OpenMP dialect oepration instead of the runtime?

flang/test/Lower/target_data.f90

11 ↗

(On Diff #491743)

Nit: Why are all the values captured but not used? If you don't want to capture to compare later then you can probably just do {{.*}}.

flang/lib/Lower/OpenMP.cpp

auto addOperands = [&](Fortran::lower::SymbolRef sym) {
     }
   };
   for (const Fortran::parser::OmpObject &ompObject : objectList.v) {
     Fortran::semantics::Symbol *sym = getOmpObjectSymbol(ompObject);
     addOperands(*sym);
   }
 }
 
+static mlir::Value
+getIfClauseOperand(Fortran::lower::AbstractConverter &converter,
+                   Fortran::lower::StatementContext &stmtCtx,
+                   const Fortran::parser::OmpClause::If *ifClause) {
+  fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
+  mlir::Location currentLocation = converter.getCurrentLocation();
+  auto &expr = std::get<Fortran::parser::ScalarLogicalExpr>(ifClause->v.t);
+  mlir::Value ifVal = fir::getBase(
+      converter.genExprValue(*Fortran::semantics::GetExpr(expr), stmtCtx));
+  return firOpBuilder.createConvert(currentLocation, firOpBuilder.getI1Type(),
+                                    ifVal);
+}
+
 static mlir::Type getLoopVarType(Fortran::lower::AbstractConverter &converter,
                                  std::size_t loopVarTypeSize) {
   // OpenMP runtime requires 32-bit or 64-bit loop variables.
   loopVarTypeSize = loopVarTypeSize * 8;
   if (loopVarTypeSize < 32) {
     loopVarTypeSize = 32;
   } else if (loopVarTypeSize > 64) {
     loopVarTypeSize = 64;
createBodyOfOp(Op &op, Fortran::lower::AbstractConverter &converter,
 
   if constexpr (std::is_same_v<Op, omp::ParallelOp>) {
     threadPrivatizeVars(converter, eval);
     if (clauses)
       genCopyinClause(converter, *clauses);
   }
 }
 
+static void
+createTargetDataOp(Fortran::lower::AbstractConverter &converter,
+                   const Fortran::parser::OmpClauseList &opClauseList,
+                   const llvm::omp::Directive &directive) {
+  Fortran::lower::StatementContext stmtCtx;
+  auto currentLocation = converter.getCurrentLocation();
+  auto &firOpBuilder = converter.getFirOpBuilder();
+
+  Value ifClauseOperand, deviceOperand;
+  UnitAttr nowaitAttr;
+  SmallVector<Value> useDevicePtrOperand, useDeviceAddrOperand, mapOperands;
+  SmallVector<IntegerAttr> mapTypes;
+
+  auto addMapClause = [&firOpBuilder, &currentLocation, &converter,
+                       &mapOperands, &mapTypes](const auto &mapClause) {
+    auto mapType = std::get<Fortran::parser::OmpMapType::Type>(
+        std::get<std::optional<Fortran::parser::OmpMapType>>(mapClause->v.t)
+            ->t);
+    llvm::omp::OpenMPOffloadMappingFlags mapTypeBits =
+        llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE;
+    switch (mapType) {
+    case Fortran::parser::OmpMapType::Type::To:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO;
+      break;
+    case Fortran::parser::OmpMapType::Type::From:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
+      break;
+    case Fortran::parser::OmpMapType::Type::Tofrom:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO |
+                     llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
+      break;
+    case Fortran::parser::OmpMapType::Type::Alloc:
+    case Fortran::parser::OmpMapType::Type::Release:
+      // alloc and release is the default behavior in the runtime library,
+      // i.e. if we don't pass any bits alloc/release that is what the
+      // runtime is going to do. Therefore, we don't need to signal anything
+      // for these two type modifiers.
+      break;
+    case Fortran::parser::OmpMapType::Type::Delete:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_DELETE;
+    }
+    if (std::get<std::optional<Fortran::parser::OmpMapType::Always>>(
+            std::get<std::optional<Fortran::parser::OmpMapType>>(mapClause->v.t)
+                ->t)
+            .has_value()) {
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS;
+    }
+    // TODO: Add support MapTypeModifiers close, mapper, present, iterator
+    auto mapTypeVal = firOpBuilder.getIntegerAttr(
+        firOpBuilder.getI64Type(),
+        static_cast<
+            std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>(
+            mapTypeBits));
+
+    SmallVector<Value> mapOperand;
+    genObjectList(std::get<Fortran::parser::OmpObjectList>(mapClause->v.t),
+                  converter, mapOperand);
+
+    for (auto mapOp : mapOperand) {
+      mapOperands.push_back(mapOp);
+      mapTypes.push_back(mapTypeVal);
+    }
+  };
+
+  for (const Fortran::parser::OmpClause &clause : opClauseList.v) {
+    if (const auto &ifClause =
+            std::get_if<Fortran::parser::OmpClause::If>(&clause.u)) {
+      ifClauseOperand = getIfClauseOperand(converter, stmtCtx, ifClause);
+    } else if (const auto &deviceClause =
+                   std::get_if<Fortran::parser::OmpClause::Device>(&clause.u)) {
+      if (auto deviceModifier = std::get<
+              std::optional<Fortran::parser::OmpDeviceClause::DeviceModifier>>(
+              deviceClause->v.t)) {
+        if (deviceModifier ==
+            Fortran::parser::OmpDeviceClause::DeviceModifier::Ancestor) {
+          TODO(currentLocation, "OMPD_target Device Modifier Ancestor");
+        }
+      }
+      if (const auto *deviceExpr = Fortran::semantics::GetExpr(
+              std::get<Fortran::parser::ScalarIntExpr>(deviceClause->v.t))) {
+        deviceOperand =
+            fir::getBase(converter.genExprValue(*deviceExpr, stmtCtx));
+      }
+    } else if (std::get_if<Fortran::parser::OmpClause::UseDevicePtr>(
+                   &clause.u)) {
+      TODO(currentLocation, "OMPD_target Use Device Ptr");
+    } else if (std::get_if<Fortran::parser::OmpClause::UseDeviceAddr>(
+                   &clause.u)) {
+      TODO(currentLocation, "OMPD_target Use Device Addr");
+    } else if (std::get_if<Fortran::parser::OmpClause::Nowait>(&clause.u)) {
+      nowaitAttr = firOpBuilder.getUnitAttr();
+    } else if (const auto &mapClause =
+                   std::get_if<Fortran::parser::OmpClause::Map>(&clause.u)) {
+      addMapClause(mapClause);
+    }
+  }
+
+  SmallVector<Attribute> mapTypesAttr(mapTypes.begin(), mapTypes.end());
+
+  if (directive == llvm::omp::Directive::OMPD_target_data) {
+    firOpBuilder.create<omp::DataOp>(
+        currentLocation, ifClauseOperand, deviceOperand, useDevicePtrOperand,
+        useDeviceAddrOperand, mapOperands,
+        ArrayAttr::get(firOpBuilder.getContext(), mapTypesAttr));
+  } else if (directive == llvm::omp::Directive::OMPD_target_enter_data) {
+    firOpBuilder.create<omp::EnterDataOp>(
+        currentLocation, ifClauseOperand, deviceOperand, nowaitAttr,
+        mapOperands, ArrayAttr::get(firOpBuilder.getContext(), mapTypesAttr));
+  } else if (directive == llvm::omp::Directive::OMPD_target_exit_data) {
+    firOpBuilder.create<omp::ExitDataOp>(
+        currentLocation, ifClauseOperand, deviceOperand, nowaitAttr,
+        mapOperands, ArrayAttr::get(firOpBuilder.getContext(), mapTypesAttr));
+  }
+}
+
 static void genOMP(Fortran::lower::AbstractConverter &converter,
                    Fortran::lower::pft::Evaluation &eval,
                    const Fortran::parser::OpenMPSimpleStandaloneConstruct
                        &simpleStandaloneConstruct) {
   const auto &directive =
       std::get<Fortran::parser::OmpSimpleStandaloneDirective>(
           simpleStandaloneConstruct.t);
+  auto currentLocation = converter.getCurrentLocation();
+  auto &firOpBuilder = converter.getFirOpBuilder();
+  const auto &opClauseList =
+      std::get<Fortran::parser::OmpClauseList>(simpleStandaloneConstruct.t);
+
   switch (directive.v) {
   default:
     break;
   case llvm::omp::Directive::OMPD_barrier:
-    converter.getFirOpBuilder().create<mlir::omp::BarrierOp>(
-        converter.getCurrentLocation());
+    firOpBuilder.create<omp::BarrierOp>(currentLocation);
     break;
   case llvm::omp::Directive::OMPD_taskwait:
-    converter.getFirOpBuilder().create<mlir::omp::TaskwaitOp>(
-        converter.getCurrentLocation());
+    firOpBuilder.create<omp::TaskwaitOp>(currentLocation);
     break;
   case llvm::omp::Directive::OMPD_taskyield:
-    converter.getFirOpBuilder().create<mlir::omp::TaskyieldOp>(
-        converter.getCurrentLocation());
+    firOpBuilder.create<omp::TaskyieldOp>(currentLocation);
     break;
+  case llvm::omp::Directive::OMPD_target_data:
   case llvm::omp::Directive::OMPD_target_enter_data:
-    TODO(converter.getCurrentLocation(), "OMPD_target_enter_data");
   case llvm::omp::Directive::OMPD_target_exit_data:
-    TODO(converter.getCurrentLocation(), "OMPD_target_exit_data");
+    createTargetDataOp(converter, opClauseList, directive.v);
+    break;
   case llvm::omp::Directive::OMPD_target_update:
-    TODO(converter.getCurrentLocation(), "OMPD_target_update");
+    TODO(currentLocation, "OMPD_target_update");
   case llvm::omp::Directive::OMPD_ordered:
-    TODO(converter.getCurrentLocation(), "OMPD_ordered");
+    TODO(currentLocation, "OMPD_ordered");
   }
 }
 
 static void
 genAllocateClause(Fortran::lower::AbstractConverter &converter,
                   const Fortran::parser::OmpAllocateClause &ompAllocateClause,
                   SmallVector<Value> &allocatorOperands,
                   SmallVector<Value> &allocateOperands) {
case Fortran::parser::OmpProcBindClause::Type::Spread:
     break;
   case Fortran::parser::OmpProcBindClause::Type::Primary:
     pbKind = omp::ClauseProcBindKind::Primary;
     break;
   }
   return omp::ClauseProcBindKindAttr::get(firOpBuilder.getContext(), pbKind);
 }
 
-static mlir::Value
-getIfClauseOperand(Fortran::lower::AbstractConverter &converter,
-                   Fortran::lower::StatementContext &stmtCtx,
-                   const Fortran::parser::OmpClause::If *ifClause) {
-  fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
-  mlir::Location currentLocation = converter.getCurrentLocation();
-  auto &expr = std::get<Fortran::parser::ScalarLogicalExpr>(ifClause->v.t);
-  mlir::Value ifVal = fir::getBase(
-      converter.genExprValue(*Fortran::semantics::GetExpr(expr), stmtCtx));
-  return firOpBuilder.createConvert(currentLocation, firOpBuilder.getI1Type(),
-                                    ifVal);
-}
-
 /* When parallel is used in a combined construct, then use this function to
  * create the parallel operation. It handles the parallel specific clauses
  * and leaves the rest for handling at the inner operations.
  * TODO: Refactor clause handling
  */
 template <typename Directive>
 static void
 createCombinedParallelOp(Fortran::lower::AbstractConverter &converter,

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp

if (curOp.getAccumulator().getType().isa<MemRefType>()) {
       // TODO: Support memref type in variable operands
       return rewriter.notifyMatchFailure(curOp, "memref is not supported yet");
     }
     rewriter.replaceOpWithNewOp<omp::ReductionOp>(
         curOp, TypeRange(), adaptor.getOperands(), curOp->getAttrs());
     return success();
   }
 };
+
+template <typename Op>
+struct LegalizeDataOpForLLVMTranslation : public ConvertOpToLLVMPattern<Op> {
+  using ConvertOpToLLVMPattern<Op>::ConvertOpToLLVMPattern;
+  LogicalResult
+  matchAndRewrite(Op curOp, typename Op::Adaptor adaptor,
+                  ConversionPatternRewriter &rewriter) const override {
+    rewriter.replaceOpWithNewOp<Op>(curOp, TypeRange(), adaptor.getOperands(),
+                                    curOp.getOperation()->getAttrs());
+    return success();
+  }
+};
 } // namespace
 
 void mlir::configureOpenMPToLLVMConversionLegality(
     ConversionTarget &target, LLVMTypeConverter &typeConverter) {
   target.addDynamicallyLegalOp<mlir::omp::CriticalOp, mlir::omp::ParallelOp,
                                mlir::omp::WsLoopOp, mlir::omp::SimdLoopOp,
                                mlir::omp::MasterOp, mlir::omp::SectionsOp,
                                mlir::omp::SingleOp>([&](Operation *op) {
     return typeConverter.isLegal(&op->getRegion(0)) &&
            typeConverter.isLegal(op->getOperandTypes()) &&
            typeConverter.isLegal(op->getResultTypes());
   });
-  target
-      .addDynamicallyLegalOp<mlir::omp::AtomicReadOp, mlir::omp::AtomicWriteOp,
-                             mlir::omp::FlushOp, mlir::omp::ThreadprivateOp>(
-          [&](Operation *op) {
-            return typeConverter.isLegal(op->getOperandTypes()) &&
-                   typeConverter.isLegal(op->getResultTypes());
-          });
+  target.addDynamicallyLegalOp<mlir::omp::AtomicReadOp,
+                               mlir::omp::AtomicWriteOp, mlir::omp::FlushOp,
+                               mlir::omp::ThreadprivateOp, mlir::omp::DataOp,
+                               mlir::omp::EnterDataOp, mlir::omp::ExitDataOp>(
   target.addDynamicallyLegalOp<mlir::omp::ReductionOp>([&](Operation *op) {
     return typeConverter.isLegal(op->getOperandTypes());
   });
 }
 
 void mlir::populateOpenMPToLLVMConversionPatterns(LLVMTypeConverter &converter,
                                                   RewritePatternSet &patterns) {
   patterns.add<
       ReductionOpConversion, RegionOpConversion<omp::CriticalOp>,
       RegionOpConversion<omp::MasterOp>, ReductionOpConversion,
       RegionOpConversion<omp::MasterOp>, RegionOpConversion<omp::ParallelOp>,
       RegionOpConversion<omp::WsLoopOp>, RegionOpConversion<omp::SectionsOp>,
       RegionOpConversion<omp::SimdLoopOp>, RegionOpConversion<omp::SingleOp>,
       RegionLessOpWithVarOperandsConversion<omp::AtomicReadOp>,
       RegionLessOpWithVarOperandsConversion<omp::AtomicWriteOp>,
       RegionLessOpWithVarOperandsConversion<omp::FlushOp>,
-      RegionLessOpWithVarOperandsConversion<omp::ThreadprivateOp>>(converter);
+      RegionLessOpWithVarOperandsConversion<omp::ThreadprivateOp>,
+      LegalizeDataOpForLLVMTranslation<omp::DataOp>,
+      LegalizeDataOpForLLVMTranslation<omp::EnterDataOp>,
+      LegalizeDataOpForLLVMTranslation<omp::ExitDataOp>>(converter);
 }
 
 namespace {
 struct ConvertOpenMPToLLVMPass
     : public impl::ConvertOpenMPToLLVMBase<ConvertOpenMPToLLVMPass> {
   void runOnOperation() override;
 };
 } // namespace