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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

Great work! I only have some nits.



================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2521
+static Optional<SimpleAtomicMatch>
+matchAtomicBinaryOp(AtomicRMWOp atomicOp, OpType op, LLVM::AtomicBinOp binOp) {
+  Value lhs = op.lhs();
----------------
I'd appreciate some documentation on this function and below.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2618
+    auto type = atomicOp.getMemRefType();
+    auto resultType = lowering.convertType(simpleMatch->val.getType());
+    auto dataPtr = getDataPtr(op->getLoc(), type, adaptor.memref(),
----------------
Type conversion may fail and return null, please check the result.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2621
+                              adaptor.indices(), rewriter, getModule());
+    rewriter.create<LLVM::AtomicRMWOp>(
+        op->getLoc(), resultType, simpleMatch->binOp, dataPtr, simpleMatch->val,
----------------
Nit:` rewriter.replaceOpWithNewOp<LLVM::AtomicRMWOp>(op, resultType, ...);`


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2656
+///
+struct AtomicCmpXchgOpLowering : public LoadStoreOpLowering<AtomicRMWOp> {
+  using Base::Base;
----------------
nicolasvasilache wrote:
> flaub wrote:
> > nicolasvasilache wrote:
> > > This seems like it'd be better a `loop.while` + progressive lowering? 
> > I agree that it'd be a lot nicer to write the loop at a higher level, but it wasn't clear to me where this would go. Also, the determination of whether to use a cmpxchg or atomic_rmw is really specific to LLVM lowering. So we wouldn't want to use a loop in the case of simple bodies that can map to a single intrinsic/op at the lower level. I'm also thinking about how we will want to have a lowering from std to SPIR-V or OpenMP, in which case a loop may or may not make sense for those lowerings.
> Not for this revision but in the future there are some nice tricks we could play here.
> Your Conversion could very well decide to introduce a higher level loop construct, rewrite the region into that and let the rewrite infrastructure stitch the pieces together.
> In other words, I wasn't advocating that the atomic behavior should leak into the other targets, but that you could decide during lowering to introduce a higher level construct that is implemented and tested independently.
This is similar to the discussion @nicolasvasilache and I had about memory copies, and is worse discussing in general. One practical thing I'd like to point out: we need to make sure we don't introduce cyclic library dependencies, and it may be tricky.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2713
+    // Conditionally branch to the end or back to the loop depending on %ok.
+    std::array<Value, 1> condBrProperOperands{ok.res()};
+    std::array<Block *, 2> condBrDestinations{endBlock, loopBlock};
----------------
Nit: normally, single-result ops should be convertible to `Value` so you shouldn't be needing the `.res()` part


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74401