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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74401