[PATCH] D78753: [MLIR] Add conversion from AtomicRMWOp -> GenericAtomicRMWOp.

[Alexander Belyaev via Phabricator via llvm-commits]

pifon2a marked 7 inline comments as done.
pifon2a added inline comments.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:2657
+/// }
+struct AtomicToGenericAtomicRMWOp : public OpConversionPattern<AtomicRMWOp> {
+public:
----------------
herhut wrote:
> This pattern does not have a test.
> 
> Also, maybe call it `AtomicMinfMaxfTo...` as that is what it is doing.
It was tested through lowering to `cmpxchg`. Now there is a separate test for this very pattern.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:2877
+static void
+populateStdToStdConversionPatterns(LLVMTypeConverter &converter,
+                                   OwningRewritePatternList &patterns) {
----------------
herhut wrote:
> ftynse wrote:
> > herhut wrote:
> > > Expose this from a the `ConvertStandardToStandard` header file with a descriptive name.
> > Yes, I was thinking that this pattern is not at all concerned with LLVM and should not live in "StandardToLLVM". However, the decision when to apply this pattern is driven by what LLVM supports.
> > 
> > Also, ConvertStandardToStandard feels weird to me. lib/Conversion was originally intended to solve the problem of the A->B conversion depending both on A dialect and B dialect so making little sense in either Dialect/A or Dialect/B... I would rather have it in `lib/Dialect/StandardOps/Transforms/ExpandAtomic` and call it from the LLVM conversion if necessary. If we ever have non-LLVM users or want to use the expansion for other cases than we currently do, it will be easier to extend. 
> Thinking of it, we do the same in the GPU dialect where we have the `AllReduceOpLowering` that also lives in `Transforms`. So +1.
Should we also clean-up `mlir/lib/Conversion/StandardToStandard/StandardToStandard.cpp` and move it to `StandardOps/Transforms`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78753