Skip to content

Conversation

@matthias-springer
Copy link
Member

When folding an op during a conversion, first try to legalize all generated constants, then replace the original operation. This is slightly more efficient because fewer rewrites must be rolled back in case a generated constant could not be legalized.

Note: This is in preparation of the One-Shot Dialect Conversion refactoring.

@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Apr 4, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 4, 2025

@llvm/pr-subscribers-mlir-core

Author: Matthias Springer (matthias-springer)

Changes

When folding an op during a conversion, first try to legalize all generated constants, then replace the original operation. This is slightly more efficient because fewer rewrites must be rolled back in case a generated constant could not be legalized.

Note: This is in preparation of the One-Shot Dialect Conversion refactoring.


Full diff: https://github.com/llvm/llvm-project/pull/134384.diff

1 Files Affected:

  • (modified) mlir/lib/Transforms/Utils/DialectConversion.cpp (+3-3)
diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 444c505b64232..0b342a0e5d4bc 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -2109,9 +2109,6 @@ OperationLegalizer::legalizeWithFold(Operation *op,
   if (replacementValues.empty())
     return legalize(op, rewriter);
 
-  // Insert a replacement for 'op' with the folded replacement values.
-  rewriter.replaceOp(op, replacementValues);
-
   // Recursively legalize any new constant operations.
   for (unsigned i = curState.numRewrites, e = rewriterImpl.rewrites.size();
        i != e; ++i) {
@@ -2128,6 +2125,9 @@ OperationLegalizer::legalizeWithFold(Operation *op,
     }
   }
 
+  // Insert a replacement for 'op' with the folded replacement values.
+  rewriter.replaceOp(op, replacementValues);
+
   LLVM_DEBUG(logSuccess(rewriterImpl.logger, ""));
   return success();
 }

@llvmbot
Copy link
Member

llvmbot commented Apr 4, 2025

@llvm/pr-subscribers-mlir

Author: Matthias Springer (matthias-springer)

Changes

When folding an op during a conversion, first try to legalize all generated constants, then replace the original operation. This is slightly more efficient because fewer rewrites must be rolled back in case a generated constant could not be legalized.

Note: This is in preparation of the One-Shot Dialect Conversion refactoring.


Full diff: https://github.com/llvm/llvm-project/pull/134384.diff

1 Files Affected:

  • (modified) mlir/lib/Transforms/Utils/DialectConversion.cpp (+3-3)
diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 444c505b64232..0b342a0e5d4bc 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -2109,9 +2109,6 @@ OperationLegalizer::legalizeWithFold(Operation *op,
   if (replacementValues.empty())
     return legalize(op, rewriter);
 
-  // Insert a replacement for 'op' with the folded replacement values.
-  rewriter.replaceOp(op, replacementValues);
-
   // Recursively legalize any new constant operations.
   for (unsigned i = curState.numRewrites, e = rewriterImpl.rewrites.size();
        i != e; ++i) {
@@ -2128,6 +2125,9 @@ OperationLegalizer::legalizeWithFold(Operation *op,
     }
   }
 
+  // Insert a replacement for 'op' with the folded replacement values.
+  rewriter.replaceOp(op, replacementValues);
+
   LLVM_DEBUG(logSuccess(rewriterImpl.logger, ""));
   return success();
 }

Copy link
Member

@zero9178 zero9178 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@matthias-springer matthias-springer merged commit 68ce637 into main Apr 5, 2025
14 checks passed
@matthias-springer matthias-springer deleted the users/matthias-springer/dialect_conv_replace_later branch April 5, 2025 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mlir:core MLIR Core Infrastructure mlir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants