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

[Shraiysh via Phabricator via cfe-commits]

shraiysh added a comment.

Thanks for the review @peixin.



================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:513
+
+def AtomicReadOp : OpenMP_Op<"atomic.read"> {
+  let arguments = (ins OpenMP_PointerLikeType:$address,
----------------
peixin wrote:
> 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. 
Yes - there will be a kmpc_flush call, but OpenMP Dialect does not have to worry about that. OpenMP IR Builder takes care of flushes in the function [[ https://llvm.org/doxygen/classllvm_1_1OpenMPIRBuilder.html#a388d5a62753f4e7ff4b72e54c1233fbc | createAtomicRead ]].


================
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(
----------------
peixin wrote:
> Please also check OpenMP 5.1 Spec. It seems that `acq_rel` is allowed if read clause is specified.
Thanks for this observation, I will update it with the 5.1 standard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992