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

[Frank Laub via Phabricator via llvm-commits]

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


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2656
+///
+struct AtomicCmpXchgOpLowering : public LoadStoreOpLowering<AtomicRMWOp> {
+  using Base::Base;
----------------
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.


================
Comment at: mlir/test/Conversion/StandardToLLVM/convert-to-llvmir.mlir:869
+  atomic_rmw %loaded = %I[%i] : memref<10xi32> {
+    %0 = addi %loaded, %ival : i32
+    atomic_rmw_yield %0 : i32
----------------
nicolasvasilache wrote:
> So this is interesting to me.
> Weren't you and/or @jbruestle advocating that we should have the reduction op encoded as an attribute in the case or affine.parallel_for with reduction semantics?
> It seems a very similar scenario to me, I'd be interested of where you draw the distinction between "encoded as an attribute" and just use a region?
I think we initially thought having a closed attribute would be good, but it seemed that providing the ability to lower arbitrary reductions into cmpxchg wasn't too hard to do and it was easy enough to identify these simple cases that do lower to a single intrinsic. Our current plan is to still use an enum at the top level (the tile dialect), but then use a region for affine and below. The upcoming `affine.atomic_rmw` should basically mirror the standard one in regards to region vs attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74401