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

[Shraiysh via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9fb52cb3f123: [MLIR][OpenMP] Added omp.atomic.read and 
omp.atomic.write (authored by shraiysh).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  flang/lib/Semantics/check-omp-structure.cpp
  llvm/include/llvm/Frontend/OpenMP/OMP.td
  mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
  mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
  mlir/test/Dialect/OpenMP/invalid.mlir
  mlir/test/Dialect/OpenMP/ops.mlir

Index: mlir/test/Dialect/OpenMP/ops.mlir
===
--- mlir/test/Dialect/OpenMP/ops.mlir
+++ mlir/test/Dialect/OpenMP/ops.mlir
@@ -454,3 +454,37 @@
 
   return
 }
+
+// CHECK-LABEL: omp_atomic_read
+// CHECK-SAME: (%[[ADDR:.*]]: memref)
+func @omp_atomic_read(%addr : memref) {
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] : memref -> i32
+  %1 = omp.atomic.read %addr : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(seq_cst) : memref -> i32
+  %2 = omp.atomic.read %addr memory_order(seq_cst) : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(acquire) : memref -> i32
+  %5 = omp.atomic.read %addr memory_order(acquire) : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(relaxed) : memref -> i32
+  %6 = omp.atomic.read %addr memory_order(relaxed) : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] hint(contended, nonspeculative) : memref -> i32
+  %7 = omp.atomic.read %addr hint(nonspeculative, contended) : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(seq_cst) hint(contended, speculative) : memref -> i32
+  %8 = omp.atomic.read %addr hint(speculative, contended) memory_order(seq_cst) : memref -> i32
+  return
+}
+
+// CHECK-LABEL: omp_atomic_write
+// CHECK-SAME: (%[[ADDR:.*]]: memref, %[[VAL:.*]]: i32)
+func @omp_atomic_write(%addr : memref, %val : i32) {
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] : memref, i32
+  omp.atomic.write %addr, %val : memref, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] memory_order(seq_cst) : memref, i32
+  omp.atomic.write %addr, %val memory_order(seq_cst) : memref, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] memory_order(release) : memref, i32
+  omp.atomic.write %addr, %val memory_order(release) : memref, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] memory_order(relaxed) : memref, i32
+  omp.atomic.write %addr, %val memory_order(relaxed) : memref, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] hint(uncontended, speculative) : memref, i32
+  omp.atomic.write %addr, %val hint(speculative, uncontended) : memref, i32
+  return
+}
Index: mlir/test/Dialect/OpenMP/invalid.mlir
===
--- mlir/test/Dialect/OpenMP/invalid.mlir
+++ mlir/test/Dialect/OpenMP/invalid.mlir
@@ -375,3 +375,99 @@
   }
   return
 }
+
+// -
+
+func @omp_atomic_read1(%addr : memref) {
+  // expected-error @below {{the hints omp_sync_hint_nonspeculative and omp_sync_hint_speculative cannot be combined.}}
+  %1 = omp.atomic.read %addr hint(speculative, nonspeculative) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read2(%addr : memref) {
+  // expected-error @below {{attribute 'memory_order' failed to satisfy constraint: MemoryOrderKind Clause}}
+  %1 = omp.atomic.read %addr memory_order(xyz) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read3(%addr : memref) {
+  // expected-error @below {{memory-order must not be acq_rel or release for atomic reads}}
+  %1 = omp.atomic.read %addr memory_order(acq_rel) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read4(%addr : memref) {
+  // expected-error @below {{memory-order must not be acq_rel or release for atomic reads}}
+  %1 = omp.atomic.read %addr memory_order(release) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read5(%addr : memref) {
+  // expected-error @below {{at most one memory_order clause can appear on the omp.atomic.read operation}}
+  %1 = omp.atomic.read %addr memory_order(acquire) memory_order(relaxed) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read6(%addr : memref) {
+  // expected-error @below {{at most one hint clause can appear on the omp.atomic.read operation}}
+  %1 = omp.atomic.read  %addr hint(speculative) hint(contended) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_write1(%addr : memref, %val : i32) {
+  // expected-error @below {{the hints omp_sync_hint_uncontended and omp_sync_hint_contended cannot be combined}}
+  omp.atomic.write  %addr, %val hint(contended, uncontended) : memref, i32
+  return
+}
+
+// -
+
+func @omp_atomic_write2(%addr : memref, %val : i32) {
+  // expected-error @below {{memory-order must not be acq_rel or acquire for 

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

[Shraiysh via Phabricator via cfe-commits]

shraiysh updated this revision to Diff 382537.
shraiysh added a comment.

Rebase with main


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  flang/lib/Semantics/check-omp-structure.cpp
  llvm/include/llvm/Frontend/OpenMP/OMP.td
  mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
  mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
  mlir/test/Dialect/OpenMP/invalid.mlir
  mlir/test/Dialect/OpenMP/ops.mlir

Index: mlir/test/Dialect/OpenMP/ops.mlir
===
--- mlir/test/Dialect/OpenMP/ops.mlir
+++ mlir/test/Dialect/OpenMP/ops.mlir
@@ -454,3 +454,37 @@
 
   return
 }
+
+// CHECK-LABEL: omp_atomic_read
+// CHECK-SAME: (%[[ADDR:.*]]: memref)
+func @omp_atomic_read(%addr : memref) {
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] : memref -> i32
+  %1 = omp.atomic.read %addr : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(seq_cst) : memref -> i32
+  %2 = omp.atomic.read %addr memory_order(seq_cst) : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(acquire) : memref -> i32
+  %5 = omp.atomic.read %addr memory_order(acquire) : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(relaxed) : memref -> i32
+  %6 = omp.atomic.read %addr memory_order(relaxed) : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] hint(contended, nonspeculative) : memref -> i32
+  %7 = omp.atomic.read %addr hint(nonspeculative, contended) : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(seq_cst) hint(contended, speculative) : memref -> i32
+  %8 = omp.atomic.read %addr hint(speculative, contended) memory_order(seq_cst) : memref -> i32
+  return
+}
+
+// CHECK-LABEL: omp_atomic_write
+// CHECK-SAME: (%[[ADDR:.*]]: memref, %[[VAL:.*]]: i32)
+func @omp_atomic_write(%addr : memref, %val : i32) {
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] : memref, i32
+  omp.atomic.write %addr, %val : memref, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] memory_order(seq_cst) : memref, i32
+  omp.atomic.write %addr, %val memory_order(seq_cst) : memref, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] memory_order(release) : memref, i32
+  omp.atomic.write %addr, %val memory_order(release) : memref, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] memory_order(relaxed) : memref, i32
+  omp.atomic.write %addr, %val memory_order(relaxed) : memref, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] hint(uncontended, speculative) : memref, i32
+  omp.atomic.write %addr, %val hint(speculative, uncontended) : memref, i32
+  return
+}
Index: mlir/test/Dialect/OpenMP/invalid.mlir
===
--- mlir/test/Dialect/OpenMP/invalid.mlir
+++ mlir/test/Dialect/OpenMP/invalid.mlir
@@ -375,3 +375,99 @@
   }
   return
 }
+
+// -
+
+func @omp_atomic_read1(%addr : memref) {
+  // expected-error @below {{the hints omp_sync_hint_nonspeculative and omp_sync_hint_speculative cannot be combined.}}
+  %1 = omp.atomic.read %addr hint(speculative, nonspeculative) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read2(%addr : memref) {
+  // expected-error @below {{attribute 'memory_order' failed to satisfy constraint: MemoryOrderKind Clause}}
+  %1 = omp.atomic.read %addr memory_order(xyz) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read3(%addr : memref) {
+  // expected-error @below {{memory-order must not be acq_rel or release for atomic reads}}
+  %1 = omp.atomic.read %addr memory_order(acq_rel) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read4(%addr : memref) {
+  // expected-error @below {{memory-order must not be acq_rel or release for atomic reads}}
+  %1 = omp.atomic.read %addr memory_order(release) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read5(%addr : memref) {
+  // expected-error @below {{at most one memory_order clause can appear on the omp.atomic.read operation}}
+  %1 = omp.atomic.read %addr memory_order(acquire) memory_order(relaxed) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read6(%addr : memref) {
+  // expected-error @below {{at most one hint clause can appear on the omp.atomic.read operation}}
+  %1 = omp.atomic.read  %addr hint(speculative) hint(contended) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_write1(%addr : memref, %val : i32) {
+  // expected-error @below {{the hints omp_sync_hint_uncontended and omp_sync_hint_contended cannot be combined}}
+  omp.atomic.write  %addr, %val hint(contended, uncontended) : memref, i32
+  return
+}
+
+// -
+
+func @omp_atomic_write2(%addr : memref, %val : i32) {
+  // expected-error @below {{memory-order must not be acq_rel or acquire for atomic writes}}
+  omp.atomic.write  %addr, %val memory_order(acq_rel) : memref, i32
+  return
+}

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

[Shraiysh via Phabricator via cfe-commits]

shraiysh added inline comments.



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(

shraiysh wrote:
> 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.
Just to be clear, the OpenMP 5.1 specification changes atomic by a considerable 
amount (especially it breaks capture into two sub-constructs (coming as a 
separate patch)) and AFAIK we are targeting OpenMP 5.0. So, do I still update 
it to the 5.1 standard (this will affect further implementation of atomic 
construct)? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Peixin Qiao via Phabricator via cfe-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'.");

peixin wrote:
> 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.
> 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.

Sorry. Please ignore this comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Peixin Qiao via Phabricator via cfe-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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shraiysh via Phabricator via cfe-commits]

shraiysh updated this revision to Diff 381772.
shraiysh added a comment.

Rebase with main, updated syntax.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  flang/lib/Semantics/check-omp-structure.cpp
  llvm/include/llvm/Frontend/OpenMP/OMP.td
  mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
  mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
  mlir/test/Dialect/OpenMP/invalid.mlir
  mlir/test/Dialect/OpenMP/ops.mlir

Index: mlir/test/Dialect/OpenMP/ops.mlir
===
--- mlir/test/Dialect/OpenMP/ops.mlir
+++ mlir/test/Dialect/OpenMP/ops.mlir
@@ -454,3 +454,37 @@
 
   return
 }
+
+// CHECK-LABEL: omp_atomic_read
+// CHECK-SAME: (%[[ADDR:.*]]: memref)
+func @omp_atomic_read(%addr : memref) {
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] : memref -> i32
+  %1 = omp.atomic.read %addr : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(seq_cst) : memref -> i32
+  %2 = omp.atomic.read %addr memory_order(seq_cst) : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(acquire) : memref -> i32
+  %5 = omp.atomic.read %addr memory_order(acquire) : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(relaxed) : memref -> i32
+  %6 = omp.atomic.read %addr memory_order(relaxed) : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] hint(contended, nonspeculative) : memref -> i32
+  %7 = omp.atomic.read %addr hint(nonspeculative, contended) : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(seq_cst) hint(contended, speculative) : memref -> i32
+  %8 = omp.atomic.read %addr hint(speculative, contended) memory_order(seq_cst) : memref -> i32
+  return
+}
+
+// CHECK-LABEL: omp_atomic_write
+// CHECK-SAME: (%[[ADDR:.*]]: memref, %[[VAL:.*]]: i32)
+func @omp_atomic_write(%addr : memref, %val : i32) {
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] : memref, i32
+  omp.atomic.write %addr, %val : memref, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] memory_order(seq_cst) : memref, i32
+  omp.atomic.write %addr, %val memory_order(seq_cst) : memref, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] memory_order(release) : memref, i32
+  omp.atomic.write %addr, %val memory_order(release) : memref, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] memory_order(relaxed) : memref, i32
+  omp.atomic.write %addr, %val memory_order(relaxed) : memref, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] hint(uncontended, speculative) : memref, i32
+  omp.atomic.write %addr, %val hint(speculative, uncontended) : memref, i32
+  return
+}
Index: mlir/test/Dialect/OpenMP/invalid.mlir
===
--- mlir/test/Dialect/OpenMP/invalid.mlir
+++ mlir/test/Dialect/OpenMP/invalid.mlir
@@ -375,3 +375,99 @@
   }
   return
 }
+
+// -
+
+func @omp_atomic_read1(%addr : memref) {
+  // expected-error @below {{the hints omp_sync_hint_nonspeculative and omp_sync_hint_speculative cannot be combined.}}
+  %1 = omp.atomic.read %addr hint(speculative, nonspeculative) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read2(%addr : memref) {
+  // expected-error @below {{attribute 'memory_order' failed to satisfy constraint: MemoryOrderKind Clause}}
+  %1 = omp.atomic.read %addr memory_order(xyz) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read3(%addr : memref) {
+  // expected-error @below {{memory-order must not be acq_rel or release for atomic reads}}
+  %1 = omp.atomic.read %addr memory_order(acq_rel) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read4(%addr : memref) {
+  // expected-error @below {{memory-order must not be acq_rel or release for atomic reads}}
+  %1 = omp.atomic.read %addr memory_order(release) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read5(%addr : memref) {
+  // expected-error @below {{at most one memory_order clause can appear on the omp.atomic.read operation}}
+  %1 = omp.atomic.read %addr memory_order(acquire) memory_order(relaxed) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read6(%addr : memref) {
+  // expected-error @below {{at most one hint clause can appear on the omp.atomic.read operation}}
+  %1 = omp.atomic.read  %addr hint(speculative) hint(contended) : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_write1(%addr : memref, %val : i32) {
+  // expected-error @below {{the hints omp_sync_hint_uncontended and omp_sync_hint_contended cannot be combined}}
+  omp.atomic.write  %addr, %val hint(contended, uncontended) : memref, i32
+  return
+}
+
+// -
+
+func @omp_atomic_write2(%addr : memref, %val : i32) {
+  // expected-error @below {{memory-order must not be acq_rel or acquire for atomic writes}}
+  omp.atomic.write  %addr, %val memory_order(acq_rel) : memref, 

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

[Shraiysh via Phabricator via cfe-commits]

shraiysh added a comment.

In D111992#3080308 , 
@kiranchandramohan wrote:

> LGTM.
>
> My preference is the following one. If you agree then please make the change, 
> otherwise, you can keep it as is. Also, wait a couple of days to see whether 
> others have comments.
>
>   %8 = omp.atomic.read %addr hint(speculative, contended) 
> memory_order(seq_cst) : memref -> i32

Sure, will wait for other's comments. Meanwhile, I will update it with this 
syntax.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.
This revision is now accepted and ready to land.

LGTM.

My preference is the following one. If you agree then please make the change, 
otherwise, you can keep it as is. Also, wait a couple of days to see whether 
others have comments.

  %8 = omp.atomic.read %addr hint(speculative, contended) memory_order(seq_cst) 
: memref -> i32


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shraiysh via Phabricator via cfe-commits]

shraiysh added a comment.

Hi @kiranchandramohan. Thanks for the review.

>   %8 = omp.atomic.read %addr : memref -> i32 hint(speculative, 
> contended) memory_order(seq_cst)

This cannot be done because the statement ends at optional clauses. This is 
dangerous in the following situation where parser will try to match `return` as 
a clause instead of a new instruction. I had first actually tried it this way. 
Let me know if there is something that can be done to avoid this problem with 
this syntax.

  %8 = omp.atomic.read %addr : memref -> i32 memory_order(seq_cst)
  return

We can try one of remaining two. I do not have a preference between these three:

>   %8 = omp.atomic.read %addr hint(speculative, contended) 
> memory_order(seq_cst) : memref -> i32 
>
>   %8 = omp.atomic.read %addr {hint(speculative, contended) 
> memory_order(seq_cst)} : memref -> i32

  %8 = omp.atomic.read hint(speculative, contended) memory_order(seq_cst) %addr 
: memref -> i32 // current syntax


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

The patch looks OK. I just wanted to discuss the syntax also. Would any of the 
following be better?

  %8 = omp.atomic.read %addr : memref -> i32 hint(speculative, contended) 
memory_order(seq_cst) 

  %8 = omp.atomic.read %addr hint(speculative, contended) memory_order(seq_cst) 
: memref -> i32 

  %8 = omp.atomic.read %addr {hint(speculative, contended) 
memory_order(seq_cst)} : memref -> i32


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shraiysh via Phabricator via cfe-commits]

shraiysh added a comment.

Hi all, the NFC has been merged, and the build is passing. This is ready for 
review now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shraiysh via Phabricator via cfe-commits]

shraiysh updated this revision to Diff 380718.
shraiysh added a comment.
Herald added projects: clang, Flang.
Herald added a subscriber: cfe-commits.

Rebase with main


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  flang/lib/Semantics/check-omp-structure.cpp
  llvm/include/llvm/Frontend/OpenMP/OMP.td
  mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
  mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
  mlir/test/Dialect/OpenMP/invalid.mlir
  mlir/test/Dialect/OpenMP/ops.mlir

Index: mlir/test/Dialect/OpenMP/ops.mlir
===
--- mlir/test/Dialect/OpenMP/ops.mlir
+++ mlir/test/Dialect/OpenMP/ops.mlir
@@ -414,3 +414,35 @@
   }
   return
 }
+
+// CHECK-LABEL: omp_atomic_read
+func @omp_atomic_read(%addr : memref) {
+  // CHECK: %{{.*}} = omp.atomic.read %{{.*}} : memref -> i32
+  %1 = omp.atomic.read %addr : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read memory_order(seq_cst) %{{.*}} : memref -> i32
+  %2 = omp.atomic.read memory_order(seq_cst) %addr : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read memory_order(acquire) %{{.*}} : memref -> i32
+  %5 = omp.atomic.read memory_order(acquire) %addr : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read memory_order(relaxed) %{{.*}} : memref -> i32
+  %6 = omp.atomic.read memory_order(relaxed) %addr : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read hint(contended, nonspeculative) %{{.*}} : memref -> i32
+  %7 = omp.atomic.read hint(nonspeculative, contended) %addr : memref -> i32
+  // CHECK: %{{.*}} = omp.atomic.read memory_order(seq_cst) hint(contended, speculative) %{{.*}} : memref -> i32
+  %8 = omp.atomic.read hint(speculative, contended) memory_order(seq_cst) %addr : memref -> i32
+  return
+}
+
+// CHECK-LABEL: omp_atomic_write
+func @omp_atomic_write(%addr : memref, %val : i32) {
+  // CHECK: omp.atomic.write %{{.*}} : memref, %{{.*}} : i32
+  omp.atomic.write %addr : memref, %val : i32
+  // CHECK: omp.atomic.write memory_order(seq_cst) %{{.*}} : memref, %{{.*}} : i32
+  omp.atomic.write memory_order(seq_cst) %addr : memref, %val : i32
+  // CHECK: omp.atomic.write memory_order(release) %{{.*}} : memref, %{{.*}} : i32
+  omp.atomic.write memory_order(release) %addr : memref, %val : i32
+  // CHECK: omp.atomic.write memory_order(relaxed) %{{.*}} : memref, %{{.*}} : i32
+  omp.atomic.write memory_order(relaxed) %addr : memref, %val : i32
+  // CHECK: omp.atomic.write hint(uncontended, speculative) %{{.*}} : memref, %{{.*}} : i32
+  omp.atomic.write hint(speculative, uncontended) %addr : memref, %val : i32
+  return
+}
Index: mlir/test/Dialect/OpenMP/invalid.mlir
===
--- mlir/test/Dialect/OpenMP/invalid.mlir
+++ mlir/test/Dialect/OpenMP/invalid.mlir
@@ -345,3 +345,99 @@
 omp.terminator
   }
 }
+
+// -
+
+func @omp_atomic_read1(%addr : memref) {
+  // expected-error @below {{the hints omp_sync_hint_nonspeculative and omp_sync_hint_speculative cannot be combined.}}
+  %1 = omp.atomic.read hint(speculative, nonspeculative) %addr : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read2(%addr : memref) {
+  // expected-error @below {{attribute 'memory_order' failed to satisfy constraint: MemoryOrderKind Clause}}
+  %1 = omp.atomic.read memory_order(xyz) %addr : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read3(%addr : memref) {
+  // expected-error @below {{memory-order must not be acq_rel or release for atomic reads}}
+  %1 = omp.atomic.read memory_order(acq_rel) %addr : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read4(%addr : memref) {
+  // expected-error @below {{memory-order must not be acq_rel or release for atomic reads}}
+  %1 = omp.atomic.read memory_order(release) %addr : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read5(%addr : memref) {
+  // expected-error @below {{at most one memory_order clause can appear on the omp.atomic.read operation}}
+  %1 = omp.atomic.read memory_order(acquire) memory_order(relaxed) %addr : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_read6(%addr : memref) {
+  // expected-error @below {{at most one hint clause can appear on the omp.atomic.read operation}}
+  %1 = omp.atomic.read hint(speculative) hint(contended) %addr : memref -> i32
+  return
+}
+
+// -
+
+func @omp_atomic_write1(%addr : memref, %val : i32) {
+  // expected-error @below {{the hints omp_sync_hint_uncontended and omp_sync_hint_contended cannot be combined}}
+  omp.atomic.write hint(contended, uncontended) %addr : memref, %val : i32
+  return
+}
+
+// -
+
+func @omp_atomic_write2(%addr : memref, %val : i32) {
+  // expected-error @below {{memory-order must not be acq_rel or acquire for atomic writes}}
+  omp.atomic.write memory_order(acq_rel) %addr : memref, %val : i32
+  return
+}
+
+// -
+
+func