[PATCH] D127037: [OpenMP][IRBuilder][OpenACC] Move common code from OpenACCToLLVMIRTranslation to Utils/DirectiveToLLVMIRTranslation so that it can be used between OpenACC and OpenMP.

Sat Jul 9 03:08:03 PDT 2022

clementval added inline comments.

================
Comment at: mlir/include/mlir/Target/LLVMIR/Dialect/Utils/DirectiveToLLVMIRTranslation.h:14
+
+#ifndef MLIR_DIALECT_UTILS_COMMONOPENMPOPENACC_H
+#define MLIR_DIALECT_UTILS_COMMONOPENMPOPENACC_H
----------------
Can you change it to reflect the new name?

================
Comment at: mlir/include/mlir/Target/LLVMIR/Dialect/Utils/DirectiveToLLVMIRTranslation.h:33
+#endif // MLIR_DIALECT_UTILS_COMMONOPENMPOPENACC_H
\ No newline at end of file

----------------
Missing new line

================
Comment at: mlir/lib/Target/LLVMIR/Dialect/Utils/DirectiveToLLVMIRTranslation.cpp:288
+
+  if(isa<acc::DataOp>(op)){
+    acc::DataOp dataOp = dyn_cast<acc::DataOp>(op);
----------------
I'm not sure it makes sense to reuse the part where we process each operands. There is likely no 1 to 1 mapping between OpenMP and OpenACC so it's likely the part that will be specific for each operation. 

================
Comment at: mlir/lib/Target/LLVMIR/Dialect/Utils/DirectiveToLLVMIRTranslation.cpp:491
+} // namespace mlir
\ No newline at end of file

----------------
Missing new line.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127037