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

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis added inline comments.


================
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();
----------------
kiranchandramohan wrote:
> Nit: I think this should be an assertion.
AFAIK the `mlir op` for this only allows for one region.


================
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;
----------------
kiranchandramohan wrote:
> 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?
Hi, I am working on a patch to add `use_device_ptr` and `use_deice_addr` 
clauses. As part of that I have restructured how `mapOperands` are processed 
and separated the mlir and codegen part of it. These are already moved to 
OMPIRBuilder in that patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits