[PATCH] D146557: [MLIR][OpenMP] Refactoring createTargetData in OMPIRBuilder

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4741
+  if (IsFinished && BB->use_empty()) {
+    delete BB;
+    return;
----------------
jdoerfert wrote:
> you should not just delete BB. eraseFromParent is a better way. That said, it is also weird you would delete something in an "emit" function.
I've changed it.

The `emitBlock`, `emitBranch` and `emitIfClause` functions are ported from Clang btw.
//clang/lib/CodeGen/CGStmt.cpp//


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357-1362
+int64_t getSizeInBytes(DataLayout &DL, const mlir::Type &type) {
+  if (isa<LLVM::LLVMPointerType>(type))
+    return DL.getTypeSize(cast<LLVM::LLVMPointerType>(type).getElementType());
+
+  return -1;
+}
----------------
jdoerfert wrote:
> TIFitis wrote:
> > @jdoerfert Is this way of getting the size correct? It seems to work for basic types and arrays which is what we support for now.
> I don't know about mlir, if the DL has a "store size" use that.
> That said, the size should potentially come from the user/front-end.
Clang seems to be doing something similar by calculating typeSize from clang::Type.

In my testing this works for what we support for now which are basic arrays, scalars, etc.

For derived types, array subscripts etc. we would need something more sophisticated. I plan on updating this as we add support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557