[PATCH] D112982: [MLIR][OpenMP] Added omp.atomic.update

[Shraiysh via Phabricator via llvm-commits]

shraiysh added inline comments.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:1403
+    return parser.emitError(parser.getNameLoc())
+           << "invalid atomic bin op in atomic update\n";
+  auto attr =
----------------
kiranchandramohan wrote:
> Move to verifier (also)?
I believe this cannot be moved to the verifier, as it's value is used in the next line (while making the attribute)


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:1415-1418
+    return parser.emitError(parser.getNameLoc())
+           << "atomic update variable " << x.name
+           << " not found in the RHS of the assignment statement in an"
+              " atomic.update operation";
----------------
kiranchandramohan wrote:
> Should this be moved to the verifier (also)?
This cannot be moved to the verifier because `expr` is used in the `resolveOperand` call later and it cannot be null.


================
Comment at: mlir/test/Dialect/OpenMP/ops.mlir:538-544
+  // CHECK: omp.atomic.update %[[XBOOL]] = %[[XBOOL]] and %[[EXPRBOOL]] : memref<i1>, i1
+  omp.atomic.update %xBool = %xBool and %exprBool : memref<i1>, i1
+  // CHECK: omp.atomic.update %[[XBOOL]] = %[[XBOOL]] or %[[EXPRBOOL]] : memref<i1>, i1
+  omp.atomic.update %xBool = %xBool or %exprBool : memref<i1>, i1
+  // CHECK: omp.atomic.update %[[XBOOL]] = %[[XBOOL]] xor %[[EXPRBOOL]] : memref<i1>, i1
+  omp.atomic.update %xBool = %xBool xor %exprBool : memref<i1>, i1
+  // CHECK: omp.atomic.update %[[XBOOL]] = %[[XBOOL]] shiftr %[[EXPRBOOL]] : memref<i1>, i1
----------------
kiranchandramohan wrote:
> These could be for integers as well right? Can you add a test for regular integer types also?
This was a mistake. It was supposed to be a test for regular integers and I don't think it makes sense to right shift on one-bit integers. I have modified it for i32 now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112982