[PATCH] D78352: [MLIR] Add GenericAtomicRMWOp.

[Alexander Belyaev via Phabricator via llvm-commits]

pifon2a marked 5 inline comments as done.
pifon2a added a comment.

Thanks !



================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:502
+    that represents the value stored in `memref[indices]` before the write is
+    performed.
+
----------------
mehdi_amini wrote:
> We should mention that no side effecting operation is allowed in the region.
https://reviews.llvm.org/D78559


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:536
+      return OpBuilder(&block, block.end());
+    }
+    // The value stored in memref[ivs].
----------------
herhut wrote:
> mehdi_amini wrote:
> > This is the kind of method I expect provided by a Traits otherwise we're losing consistency across all operations with regions.
> Do you have a new trait in mind or should this be supported by the `SingleBlockImplicitTerminator` trait? The body builder makes only sense if there is a single region. 
> 
> These are somewhat widespread already. I'd rather have it as a separate cleanup.
I am happy to do the cleanup as soon as there is some consensus.


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:511
+  return success();
+}
+
----------------
flaub wrote:
> Could we add a check that all the ops in the region are side-effect free?
done! https://reviews.llvm.org/D78559


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78352