[PATCH] D74401: [MLIR] Add std.atomic_rmw op

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/Ops.td:227
+    buffer that the read and write will be performed against, as accessed by the
+    specified indicies. The arity of the indices is the rank of the memref.
+    The result represents the latest value that was stored.
----------------
typo: indicies -> indices


================
Comment at: mlir/include/mlir/Dialect/StandardOps/Ops.td:239
+
+      %cst = constant 1.0 : f32
+      %x = atomic_rmw %loaded = %I[%i] : memref<10xf32> {
----------------
Wrap these in a mlir code block


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2518
+template <typename OpType>
+Optional<SimpleAtomicMatch> matchAtomicBinaryOp(AtomicRMWOp atomicOp, OpType op,
+                                                LLVM::AtomicBinOp binOp) {
----------------
static functions should be in the global namespace and marked as `static`. Only classes should be placed within anonymous namespaces.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2551
+  auto cmpOp = dyn_cast_or_null<CmpIOp>(op.condition().getDefiningOp());
+  if (!cmpOp) {
+    return llvm::None;
----------------
Drop trivial braces.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2574
+  auto yieldOp = cast<AtomicRMWYieldOp>(terminator);
+  if (yieldOp.result().getParentRegion() != &atomicOp.body()) {
+    return SimpleAtomicMatch{LLVM::AtomicBinOp::xchg, yieldOp.result()};
----------------
Same here.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2630
+
+// Wrap a llvm.cmpxchg operation in a while loop so that the operation can be
+// retried until it succeeds in atomically storing a new value into memory.
----------------
Top-level comments should be ///


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2688
+    auto init = rewriter.create<LLVM::LoadOp>(loc, dataPtr);
+    SmallVector<Value, 1> brProperOperands;
+    SmallVector<Block *, 1> brDestinations{loopBlock};
----------------
Don't create SmallVectors for things like this. Use ArrayRef or arrays.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2716
+    SmallVector<Value, 1> condBrProperOperands{ok.res()};
+    SmallVector<Block *, 2> condBrDestinations{endBlock, loopBlock};
+    SmallVector<Value, 1> condBrRegionOperands{newLoaded.res()};
----------------
Same here.


================
Comment at: mlir/lib/Dialect/StandardOps/Ops.cpp:3019
+  Type elementType = parentOp.getMemRefType().getElementType();
+  if (elementType != op.result().getType()) {
+    return op.emitOpError() << "needs to have type " << elementType;
----------------
Remove trivial braces.

I would have expected that this could be covered by ODS constraints.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74401