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

Fri Apr 24 04:49:42 PDT 2020

herhut added a comment.

Thanks for cleaning this up!

================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:2657
+/// }
+struct AtomicToGenericAtomicRMWOp : public OpConversionPattern<AtomicRMWOp> {
+public:
----------------
This pattern does not have a test.

Also, maybe call it `AtomicMinfMaxfTo...` as that is what it is doing.

================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:2664
+                  ConversionPatternRewriter &rewriter) const final {
+    if (matchSimpleAtomicOp(op))
       return failure();
----------------
I would drop this. The pattern already checks for two specific operations. You can make it an assert if you want to ensure that the definition of matchSimpleAtomicOp and this one stay in sync. 

Also, this should be in ConvertStandardToStandard (which is possible without the dependency on matchSimpleAtomicOp).

================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:2877
+static void
+populateStdToStdConversionPatterns(LLVMTypeConverter &converter,
+                                   OwningRewritePatternList &patterns) {
----------------
Expose this from a the `ConvertStandardToStandard` header file with a descriptive name.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78753