[PATCH] D104301: [mlir][openacc] Initial translation for DataOp to LLVM IR

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:39
+static constexpr int64_t kArgSizesPos = 2;
+
 static cl::opt<bool>
----------------
If we have a struct with 3 alloca members with proper names this is not needed anymore.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2264
+                                     Function *EnclosingFunction,
+                                     unsigned TotalNbOperand) {
+  if (!updateToLocation(Loc))
----------------
Also elsewhere.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2281
+  return {ArgsBase, Args, ArgSizes};
+}
+
----------------
The types should all exist, `Int8Ptr` is a global in the `omp` namespace. If not, you can also add them to OMPKinds.def (top).

I don't think returning a SmallVector is great. I'd suggest a struct with 3 Alloca members with names or pass such a struct in and fill it. 

Alloca IP has to be passed in from the caller. The entry point is not necessarily the right place, e.g., if this is called in a parallel region.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104301