[flang-commits] [flang] a622b21 - [mlir][Transforms] Make `ConversionPatternRewriter` constructor private (#82244)

[via flang-commits]

Author: Matthias Springer
Date: 2024-02-23T10:31:55+01:00
New Revision: a622b21f4607ee787c6fe63032a849c24374882b

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

LOG: [mlir][Transforms] Make `ConversionPatternRewriter` constructor private (#82244)

`ConversionPatternRewriter` objects should not be constructed outside of
dialect conversions. Some IR modifications performed through a
`ConversionPatternRewriter` are reflected in the IR in a delayed fashion
(e.g., only when the dialect conversion is guaranteed to succeed). Using
a `ConversionPatternRewriter` outside of the dialect conversion is
incorrect API usage and can bring the IR in an inconsistent state.

Migration guide: Use `IRRewriter` instead of
`ConversionPatternRewriter`.

Added: 
    

Modified: 
    flang/lib/Frontend/FrontendActions.cpp
    mlir/include/mlir/Transforms/DialectConversion.h
    mlir/lib/Transforms/Utils/DialectConversion.cpp

Removed: 
    


################################################################################
diff  --git a/flang/lib/Frontend/FrontendActions.cpp b/flang/lib/Frontend/FrontendActions.cpp
index 44e80e946ed832..849b3c8e4dc027 100644
--- a/flang/lib/Frontend/FrontendActions.cpp
+++ b/flang/lib/Frontend/FrontendActions.cpp
@@ -177,7 +177,7 @@ static void addAMDGPUSpecificMLIRItems(mlir::ModuleOp &mlirModule,
     return;
   }
 
-  mlir::ConversionPatternRewriter builder(mlirModule.getContext());
+  mlir::IRRewriter builder(mlirModule.getContext());
   unsigned oclcABIVERsion = codeGenOpts.CodeObjectVersion;
   auto int32Type = builder.getI32Type();
 

diff  --git a/mlir/include/mlir/Transforms/DialectConversion.h b/mlir/include/mlir/Transforms/DialectConversion.h
index 2575be4cdea1ac..5c91a9498b35d4 100644
--- a/mlir/include/mlir/Transforms/DialectConversion.h
+++ b/mlir/include/mlir/Transforms/DialectConversion.h
@@ -27,6 +27,7 @@ class Block;
 class ConversionPatternRewriter;
 class MLIRContext;
 class Operation;
+struct OperationConverter;
 class Type;
 class Value;
 
@@ -657,7 +658,6 @@ struct ConversionPatternRewriterImpl;
 /// hooks.
 class ConversionPatternRewriter final : public PatternRewriter {
 public:
-  explicit ConversionPatternRewriter(MLIRContext *ctx);
   ~ConversionPatternRewriter() override;
 
   /// Apply a signature conversion to the entry block of the given region. This
@@ -764,6 +764,14 @@ class ConversionPatternRewriter final : public PatternRewriter {
   detail::ConversionPatternRewriterImpl &getImpl();
 
 private:
+  // Allow OperationConverter to construct new rewriters.
+  friend struct OperationConverter;
+
+  /// Conversion pattern rewriters must not be used outside of dialect
+  /// conversions. They apply some IR rewrites in a delayed fashion and could
+  /// bring the IR into an inconsistent state when used standalone.
+  explicit ConversionPatternRewriter(MLIRContext *ctx);
+
   // Hide unsupported pattern rewriter API.
   using OpBuilder::setListener;
 

diff  --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 635a2cb00f3888..2cdbfb78faf27a 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -627,9 +627,11 @@ class ReplaceOperationRewrite : public OperationRewrite {
 
   void cleanup() override;
 
-private:
-  friend struct OperationConverter;
+  const TypeConverter *getConverter() const { return converter; }
+
+  bool hasChangedResults() const { return changedResults; }
 
+private:
   /// An optional type converter that can be used to materialize conversions
   /// between the new and old values if necessary.
   const TypeConverter *converter;
@@ -2387,7 +2389,9 @@ enum OpConversionMode {
   /// applied to the operations on success.
   Analysis,
 };
+} // namespace
 
+namespace mlir {
 // This class converts operations to a given conversion target via a set of
 // rewrite patterns. The conversion behaves 
diff erently depending on the
 // conversion mode.
@@ -2447,7 +2451,7 @@ struct OperationConverter {
   /// *not* to be legalizable to the target.
   DenseSet<Operation *> *trackedOps;
 };
-} // namespace
+} // namespace mlir
 
 LogicalResult OperationConverter::convert(ConversionPatternRewriter &rewriter,
                                           Operation *op) {
@@ -2539,7 +2543,7 @@ OperationConverter::finalize(ConversionPatternRewriter &rewriter) {
   for (unsigned i = 0; i < rewriterImpl.rewrites.size(); ++i) {
     auto *opReplacement =
         dyn_cast<ReplaceOperationRewrite>(rewriterImpl.rewrites[i].get());
-    if (!opReplacement || !opReplacement->changedResults)
+    if (!opReplacement || !opReplacement->hasChangedResults())
       continue;
     Operation *op = opReplacement->getOperation();
     for (OpResult result : op->getResults()) {
@@ -2563,9 +2567,9 @@ OperationConverter::finalize(ConversionPatternRewriter &rewriter) {
 
       // Legalize this result.
       rewriter.setInsertionPoint(op);
-      if (failed(legalizeChangedResultType(op, result, newValue,
-                                           opReplacement->converter, rewriter,
-                                           rewriterImpl, *inverseMapping)))
+      if (failed(legalizeChangedResultType(
+              op, result, newValue, opReplacement->getConverter(), rewriter,
+              rewriterImpl, *inverseMapping)))
         return failure();
     }
   }