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

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added a comment.
This revision now requires changes to proceed.

Thanks for the update Frank! Added a few comments.



================
Comment at: mlir/include/mlir/Dialect/StandardOps/Ops.td:262
+      AtomicRMWKindAttr:$kind,
+      AnyType:$value,
+      AnyMemRef:$memref,
----------------
Should this be a more constrained type, like 'IntegerOrFloatLike'?


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2698
+    // The 'result' of the atomic_rmw op is the newly loaded value.
+    atomicOp.res().replaceAllUsesWith(newLoaded);
+
----------------
This is invalid, *all* IR mutations need to go through the rewriter. Some pattern drivers, like DialectConversion which is being used here, will undo transformations if something goes wrong. If something is done outside of the rewriter, this leads to invalid code/crashes. Seems like you want to do `rewriter.replaceOp` here.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2766
+      AtomicRMWOpLowering,
+      AtomicCmpXchgOpLowering,
       BranchOpLowering,
----------------
Can we keep this ordered?


================
Comment at: mlir/lib/Dialect/StandardOps/Ops.cpp:2919
+
+static void print(OpAsmPrinter &p, AtomicRMWOp op) {
+  p << op.getOperation()->getName() << ' ' << stringifyAtomicRMWKind(op.kind())
----------------
Could you switch to the declarative format? It will format the enum as a string instead of a keyword for now, but that is worth remove all of this parsing code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74401