[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: jplehr.

LGTM. Please wait a day before submission. Move the flang portion to a separate commit.



================
Comment at: flang/lib/Lower/OpenMP.cpp:727
     for (mlir::Value mapOp : mapOperand) {
+      auto checkType = mapOp.getType();
+      if (auto refType = checkType.dyn_cast<fir::ReferenceType>())
----------------
kiranchandramohan wrote:
> These changes should go in a separate patch.
Nit: expand auto.


================
Comment at: flang/lib/Lower/OpenMP.cpp:727-738
+      auto checkType = mapOp.getType();
+      if (auto refType = checkType.dyn_cast<fir::ReferenceType>())
+        checkType = refType.getElementType();
+      if (auto boxType = checkType.dyn_cast<fir::BoxType>())
+        checkType = boxType.getElementType();
+
+      if (!(fir::isa_std_type(checkType) ||
----------------
These changes should go in a separate patch.


================
Comment at: flang/lib/Lower/OpenMP.cpp:737
+             !checkType.cast<fir::SequenceType>().hasDynamicExtents())))
+        TODO(currentLocation, "OMPD_target_data MapOperand type not supported");
+
----------------
Since TODO will print a "Not yet implemented" message, I think we can remove the trailing "not supported suffix".


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1523
+  auto bodyCB = [&](InsertPointTy allocaIP, InsertPointTy codeGenIP) {
+    // DataOp has only one region associated with it.
+    auto &region = cast<omp::DataOp>(op).getRegion();
----------------
Nit: I think this should be an assertion.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1502-1510
+  llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
+  llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
+      findAllocaInsertPoint(builder, moduleTranslation);
+
+  struct llvm::OpenMPIRBuilder::MapperAllocas mapperAllocas;
+  SmallVector<uint64_t> mapTypeFlags;
+  SmallVector<llvm::Constant *> mapNames;
----------------
TIFitis wrote:
> kiranchandramohan wrote:
> > Can all this go into the OpenMP IRBuilder? And be passed back if necessary via the callback.
> If we decide to move processMapOperand then these can be moved along with it.
Now that we decided not to move `processMapOperand`, can this be moved to the OpenMPIRBuilder?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142914/new/

https://reviews.llvm.org/D142914