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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1365
+  // possible, or else at the end of the function.
+  void EmitBlock(BasicBlock *BB, Function *CurFn, bool IsFinished = false);
+
----------------
why are these capitalized?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4085
+    omp::RuntimeFunction *MapperFunc,
+    function_ref<InsertPointTy(InsertPointTy CodeGenIP, int BodyGenType)>
+        BodyGenCB) {
----------------
Can we not use an `int` for some "type". The 1 and 2 below are magic, use an enum and descriptive words.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4105
+    // Emit the number of elements in the offloading arrays.
+    llvm::Value *PointerNum = Builder.getInt32(Info.NumberOfPtrs);
 
----------------
No llvm::. Also elsewhere.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4727
+  Builder.ClearInsertionPoint();
+}
+
----------------
This function doesn't make sense to me. For one, I don't know what a "unreal block" is. Nor would I have expected a block with terminator to be silently not touched. It might just be a documentation issue (in the header). I would avoid duplicating the comment here again.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4785
+  EmitBlock(ContBlock, CurFn, /*IsFinished=*/true);
+}
+
----------------
CFG Utils have helpers for these things. Do we not use them on purpose?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557