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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

MLIR part LGTM with comments addressed.



================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp:132
                 SmallVector<llvm::Constant *> &names, unsigned &index,
-                llvm::AllocaInst *argsBase, llvm::AllocaInst *args,
-                llvm::AllocaInst *argSizes) {
+                SmallVector<llvm::AllocaInst *> &mapperAllocas) {
   OpenACCIRBuilder *accBuilder = moduleTranslation.getOpenMPBuilder();
----------------
We usually pass `SmallVectorImpl &` that avoids the need to fix the number of stack elements. `SmallVector` has a fixed number even if this number is computed by template deduction.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp:285
+  auto enclosingFuncOp =
+      op.getOperation()->template getParentOfType<LLVM::LLVMFuncOp>();
+  llvm::Function *enclosingFunction =
----------------
No need for `template` here.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp:291-296
+  auto *srcLocInfo = createSourceLocationInfo(*accBuilder, op);
+
+  auto *beginMapperFunc = accBuilder->getOrCreateRuntimeFunctionPtr(
+      llvm::omp::OMPRTL___tgt_target_data_begin_mapper);
+
+  auto *endMapperFunc = accBuilder->getOrCreateRuntimeFunctionPtr(
----------------
Please expand auto here. MLIR uses `auto` when the type is obvious from the RHS, e.g., in a `dyn_cast`, or when it is too long or impossible to spell out, e.g. iterators and lambdas.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp:389-391
+  auto afterDataRegion = builder.saveIP();
+
+  builder.restoreIP(currIP);
----------------
It doesn't look like the insertion point changed between where currIP was created and here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104301