[llvm-branch-commits] [mlir] 02c9050 - [mlir] Tighten access of RewritePattern methods.

Christian Sigg via llvm-branch-commits llvm-branch-commits at lists.llvm.org
Tue Dec 8 07:49:35 PST 2020


Author: Christian Sigg
Date: 2020-12-08T16:44:51+01:00
New Revision: 02c9050155dff70497b3423ae95ed7d2ab7675a8

URL: https://github.com/llvm/llvm-project/commit/02c9050155dff70497b3423ae95ed7d2ab7675a8
DIFF: https://github.com/llvm/llvm-project/commit/02c9050155dff70497b3423ae95ed7d2ab7675a8.diff

LOG: [mlir] Tighten access of RewritePattern methods.

In RewritePattern, only expose `matchAndRewrite` as a public function. `match` can be protected (but needs to be protected because we want to call it from an override of `matchAndRewrite`). `rewrite` can be private.

For classes deriving from RewritePattern, all 3 functions can be private.

Side note: I didn't understand the need for the `using RewritePattern::matchAndRewrite` in derived classes, and started poking around. They are gone now, and I think the result is (only very slightly) cleaner.

Reviewed By: ftynse

Differential Revision: https://reviews.llvm.org/D92670

Added: 
    

Modified: 
    mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
    mlir/include/mlir/IR/PatternMatch.h
    mlir/include/mlir/Transforms/DialectConversion.h

Removed: 
    


################################################################################
diff  --git a/mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h b/mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
index bf41f29749de..5b605c165be6 100644
--- a/mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
+++ b/mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
@@ -571,11 +571,9 @@ class ConvertOpToLLVMPattern : public ConvertToLLVMPattern {
                              &typeConverter.getContext(), typeConverter,
                              benefit) {}
 
-  /// Wrappers around the RewritePattern methods that pass the derived op type.
-  void rewrite(Operation *op, ArrayRef<Value> operands,
-               ConversionPatternRewriter &rewriter) const final {
-    rewrite(cast<SourceOp>(op), operands, rewriter);
-  }
+private:
+  /// Wrappers around the ConversionPattern methods that pass the derived op
+  /// type.
   LogicalResult match(Operation *op) const final {
     return match(cast<SourceOp>(op));
   }
@@ -584,6 +582,10 @@ class ConvertOpToLLVMPattern : public ConvertToLLVMPattern {
                   ConversionPatternRewriter &rewriter) const final {
     return matchAndRewrite(cast<SourceOp>(op), operands, rewriter);
   }
+  void rewrite(Operation *op, ArrayRef<Value> operands,
+               ConversionPatternRewriter &rewriter) const final {
+    rewrite(cast<SourceOp>(op), operands, rewriter);
+  }
 
   /// Rewrite and Match methods that operate on the SourceOp type. These must be
   /// overridden by the derived pattern class.
@@ -603,10 +605,6 @@ class ConvertOpToLLVMPattern : public ConvertToLLVMPattern {
     }
     return failure();
   }
-
-private:
-  using ConvertToLLVMPattern::match;
-  using ConvertToLLVMPattern::matchAndRewrite;
 };
 
 namespace LLVM {
@@ -636,6 +634,7 @@ class OneToOneConvertToLLVMPattern : public ConvertOpToLLVMPattern<SourceOp> {
   using ConvertOpToLLVMPattern<SourceOp>::ConvertOpToLLVMPattern;
   using Super = OneToOneConvertToLLVMPattern<SourceOp, TargetOp>;
 
+private:
   /// Converts the type of the result to an LLVM type, pass operands as is,
   /// preserve attributes.
   LogicalResult
@@ -655,6 +654,7 @@ class VectorConvertToLLVMPattern : public ConvertOpToLLVMPattern<SourceOp> {
   using ConvertOpToLLVMPattern<SourceOp>::ConvertOpToLLVMPattern;
   using Super = VectorConvertToLLVMPattern<SourceOp, TargetOp>;
 
+private:
   LogicalResult
   matchAndRewrite(SourceOp op, ArrayRef<Value> operands,
                   ConversionPatternRewriter &rewriter) const override {

diff  --git a/mlir/include/mlir/IR/PatternMatch.h b/mlir/include/mlir/IR/PatternMatch.h
index 0bbb2216ee7b..1739cfa4a80c 100644
--- a/mlir/include/mlir/IR/PatternMatch.h
+++ b/mlir/include/mlir/IR/PatternMatch.h
@@ -156,17 +156,6 @@ class RewritePattern : public Pattern {
 public:
   virtual ~RewritePattern() {}
 
-  /// Rewrite the IR rooted at the specified operation with the result of
-  /// this pattern, generating any new operations with the specified
-  /// builder.  If an unexpected error is encountered (an internal
-  /// compiler error), it is emitted through the normal MLIR diagnostic
-  /// hooks and the IR is left in a valid state.
-  virtual void rewrite(Operation *op, PatternRewriter &rewriter) const;
-
-  /// Attempt to match against code rooted at the specified operation,
-  /// which is the same operation code as getRootKind().
-  virtual LogicalResult match(Operation *op) const;
-
   /// Attempt to match against code rooted at the specified operation,
   /// which is the same operation code as getRootKind(). If successful, this
   /// function will automatically perform the rewrite.
@@ -183,6 +172,18 @@ class RewritePattern : public Pattern {
   /// Inherit the base constructors from `Pattern`.
   using Pattern::Pattern;
 
+  /// Attempt to match against code rooted at the specified operation,
+  /// which is the same operation code as getRootKind().
+  virtual LogicalResult match(Operation *op) const;
+
+private:
+  /// Rewrite the IR rooted at the specified operation with the result of
+  /// this pattern, generating any new operations with the specified
+  /// builder.  If an unexpected error is encountered (an internal
+  /// compiler error), it is emitted through the normal MLIR diagnostic
+  /// hooks and the IR is left in a valid state.
+  virtual void rewrite(Operation *op, PatternRewriter &rewriter) const;
+
   /// An anchor for the virtual table.
   virtual void anchor();
 };
@@ -190,12 +191,15 @@ class RewritePattern : public Pattern {
 /// OpRewritePattern is a wrapper around RewritePattern that allows for
 /// matching and rewriting against an instance of a derived operation class as
 /// opposed to a raw Operation.
-template <typename SourceOp> struct OpRewritePattern : public RewritePattern {
+template <typename SourceOp>
+class OpRewritePattern : public RewritePattern {
+public:
   /// Patterns must specify the root operation name they match against, and can
   /// also specify the benefit of the pattern matching.
   OpRewritePattern(MLIRContext *context, PatternBenefit benefit = 1)
       : RewritePattern(SourceOp::getOperationName(), benefit, context) {}
 
+private:
   /// Wrappers around the RewritePattern methods that pass the derived op type.
   void rewrite(Operation *op, PatternRewriter &rewriter) const final {
     rewrite(cast<SourceOp>(op), rewriter);

diff  --git a/mlir/include/mlir/Transforms/DialectConversion.h b/mlir/include/mlir/Transforms/DialectConversion.h
index e02cf8fe4c0a..ecbb653f7ed9 100644
--- a/mlir/include/mlir/Transforms/DialectConversion.h
+++ b/mlir/include/mlir/Transforms/DialectConversion.h
@@ -313,6 +313,30 @@ class TypeConverter {
 /// patterns of this type can only be used with the 'apply*' methods below.
 class ConversionPattern : public RewritePattern {
 public:
+  /// Return the type converter held by this pattern, or nullptr if the pattern
+  /// does not require type conversion.
+  TypeConverter *getTypeConverter() const { return typeConverter; }
+
+protected:
+  /// See `RewritePattern::RewritePattern` for information on the other
+  /// available constructors.
+  using RewritePattern::RewritePattern;
+  /// Construct a conversion pattern that matches an operation with the given
+  /// root name. This constructor allows for providing a type converter to use
+  /// within the pattern.
+  ConversionPattern(StringRef rootName, PatternBenefit benefit,
+                    TypeConverter &typeConverter, MLIRContext *ctx)
+      : RewritePattern(rootName, benefit, ctx), typeConverter(&typeConverter) {}
+  /// Construct a conversion pattern that matches any operation type. This
+  /// constructor allows for providing a type converter to use within the
+  /// pattern. `MatchAnyOpTypeTag` is just a tag to ensure that the "match any"
+  /// behavior is what the user actually desired, `MatchAnyOpTypeTag()` should
+  /// always be supplied here.
+  ConversionPattern(PatternBenefit benefit, TypeConverter &typeConverter,
+                    MatchAnyOpTypeTag tag)
+      : RewritePattern(benefit, tag), typeConverter(&typeConverter) {}
+
+private:
   /// Hook for derived classes to implement rewriting. `op` is the (first)
   /// operation matched by the pattern, `operands` is a list of the rewritten
   /// operand values that are passed to `op`, `rewriter` can be used to emit the
@@ -323,6 +347,10 @@ class ConversionPattern : public RewritePattern {
     llvm_unreachable("unimplemented rewrite");
   }
 
+  void rewrite(Operation *op, PatternRewriter &rewriter) const final {
+    llvm_unreachable("never called");
+  }
+
   /// Hook for derived classes to implement combined matching and rewriting.
   virtual LogicalResult
   matchAndRewrite(Operation *op, ArrayRef<Value> operands,
@@ -337,42 +365,17 @@ class ConversionPattern : public RewritePattern {
   LogicalResult matchAndRewrite(Operation *op,
                                 PatternRewriter &rewriter) const final;
 
-  /// Return the type converter held by this pattern, or nullptr if the pattern
-  /// does not require type conversion.
-  TypeConverter *getTypeConverter() const { return typeConverter; }
-
-protected:
-  /// See `RewritePattern::RewritePattern` for information on the other
-  /// available constructors.
-  using RewritePattern::RewritePattern;
-  /// Construct a conversion pattern that matches an operation with the given
-  /// root name. This constructor allows for providing a type converter to use
-  /// within the pattern.
-  ConversionPattern(StringRef rootName, PatternBenefit benefit,
-                    TypeConverter &typeConverter, MLIRContext *ctx)
-      : RewritePattern(rootName, benefit, ctx), typeConverter(&typeConverter) {}
-  /// Construct a conversion pattern that matches any operation type. This
-  /// constructor allows for providing a type converter to use within the
-  /// pattern. `MatchAnyOpTypeTag` is just a tag to ensure that the "match any"
-  /// behavior is what the user actually desired, `MatchAnyOpTypeTag()` should
-  /// always be supplied here.
-  ConversionPattern(PatternBenefit benefit, TypeConverter &typeConverter,
-                    MatchAnyOpTypeTag tag)
-      : RewritePattern(benefit, tag), typeConverter(&typeConverter) {}
-
 protected:
   /// An optional type converter for use by this pattern.
   TypeConverter *typeConverter = nullptr;
-
-private:
-  using RewritePattern::rewrite;
 };
 
 /// OpConversionPattern is a wrapper around ConversionPattern that allows for
 /// matching and rewriting against an instance of a derived operation class as
 /// opposed to a raw Operation.
 template <typename SourceOp>
-struct OpConversionPattern : public ConversionPattern {
+class OpConversionPattern : public ConversionPattern {
+public:
   OpConversionPattern(MLIRContext *context, PatternBenefit benefit = 1)
       : ConversionPattern(SourceOp::getOperationName(), benefit, context) {}
   OpConversionPattern(TypeConverter &typeConverter, MLIRContext *context,
@@ -380,6 +383,7 @@ struct OpConversionPattern : public ConversionPattern {
       : ConversionPattern(SourceOp::getOperationName(), benefit, typeConverter,
                           context) {}
 
+private:
   /// Wrappers around the ConversionPattern methods that pass the derived op
   /// type.
   void rewrite(Operation *op, ArrayRef<Value> operands,
@@ -409,9 +413,6 @@ struct OpConversionPattern : public ConversionPattern {
     rewrite(op, operands, rewriter);
     return success();
   }
-
-private:
-  using ConversionPattern::matchAndRewrite;
 };
 
 /// Add a pattern to the given pattern list to convert the signature of a FuncOp


        


More information about the llvm-branch-commits mailing list