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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1487
+  // possible, or else at the end of the function.
+  void emitBlock(BasicBlock *BB, Function *CurFn, bool IsFinished = false);
+
----------------
This does not mention the deletion stuff, etc.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:2009
 
+  enum BodyGenTy { Priv, DupNoPriv, NoPriv };
+
----------------
documentation, what do they mean


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4155
+    Constant *SrcLocStr = getOrCreateSrcLocStr(Loc, SrcLocStrSize);
+    Value *srcLocInfo = getOrCreateIdent(SrcLocStr, SrcLocStrSize);
+
----------------
Style, also elsewhere.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4741
+  if (IsFinished && BB->use_empty()) {
+    delete BB;
+    return;
----------------
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.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4758-4760
+  if (isa<ConstantInt>(Cond)) {
+    bool CondConstant =
+        cast<ConstantInt>(Cond)->getUniqueInteger().getSExtValue();
----------------



================
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;
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557