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

[Frank Laub via Phabricator via llvm-commits]

flaub marked 4 inline comments as done.
flaub added inline comments.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/Ops.td:262
+      AtomicRMWKindAttr:$kind,
+      AnyType:$value,
+      AnyMemRef:$memref,
----------------
rriddle wrote:
> Should this be a more constrained type, like 'IntegerOrFloatLike'?
OK, I suppose that will work here for now. I could see this being expanded later if the lowering supported more types (which in theory it could by lowering to `cmpxchg` or others).


================
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);
+
----------------
rriddle wrote:
> 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.
OK, thanks for clarifying. I'll try to use `rewriter.replaceOp` instead.


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


================
Comment at: mlir/lib/Dialect/StandardOps/Ops.cpp:2919
+
+static void print(OpAsmPrinter &p, AtomicRMWOp op) {
+  p << op.getOperation()->getName() << ' ' << stringifyAtomicRMWKind(op.kind())
----------------
rriddle wrote:
> 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.
Cool, I will try that, thanks (sounds like a worthwhile tradeoff).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74401