[PATCH] D111992: [MLIR][OpenMP] Added omp.atomic.read and omp.atomic.write

[Peixin Qiao via Phabricator via llvm-commits]

peixin added inline comments.


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5993
   case OMPC_adjust_args:
+  case OMPC_memory_order:
     llvm_unreachable("Clause is not allowed in 'omp atomic'.");
----------------
The memory_order clause in clang side is not handled in this patch. Maybe leaving the error is better since users will know it is not supported yet in clang.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:513
+
+def AtomicReadOp : OpenMP_Op<"atomic.read"> {
+  let arguments = (ins OpenMP_PointerLikeType:$address,
----------------
How do you plan to handle synchronization between the threads if there is no region in atomic read op? Will there be one implicit `kmpc_flush` function call before `!$omp end atomic`? If yes, it is easier to find to location of emiting `kmpc_flush` if we have one region here. Please let me know if I am wrong. 


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:1246
+    StringRef memOrder = op.memory_order().getValue();
+    if (memOrder.equals("acq_rel") || memOrder.equals("release"))
+      return op.emitError(
----------------
Please also check OpenMP 5.1 Spec. It seems that `acq_rel` is allowed if read clause is specified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992