-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mlir][Transforms][NFC] Decouple ConversionPatternRewriterImpl
from ConversionPatternRewriter
#82333
Merged
matthias-springer
merged 1 commit into
main
from
users/matthias-springer/decouple_conversion_rewriter
Feb 23, 2024
Merged
[mlir][Transforms][NFC] Decouple ConversionPatternRewriterImpl
from ConversionPatternRewriter
#82333
matthias-springer
merged 1 commit into
main
from
users/matthias-springer/decouple_conversion_rewriter
Feb 23, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-core Author: Matthias Springer (matthias-springer) Changes
Full diff: https://github.com/llvm/llvm-project/pull/82333.diff 1 Files Affected:
diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 30fc2298b3deb3..ec97a4247658b8 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -725,10 +725,9 @@ static RewriteTy *findSingleRewrite(R &&rewrites, Block *block) {
namespace mlir {
namespace detail {
struct ConversionPatternRewriterImpl : public RewriterBase::Listener {
- explicit ConversionPatternRewriterImpl(PatternRewriter &rewriter,
+ explicit ConversionPatternRewriterImpl(MLIRContext *ctx,
const ConversionConfig &config)
- : rewriter(rewriter), eraseRewriter(rewriter.getContext()),
- config(config) {}
+ : eraseRewriter(ctx), config(config) {}
//===--------------------------------------------------------------------===//
// State Management
@@ -823,8 +822,8 @@ struct ConversionPatternRewriterImpl : public RewriterBase::Listener {
Type origOutputType,
const TypeConverter *converter);
- Value buildUnresolvedArgumentMaterialization(PatternRewriter &rewriter,
- Location loc, ValueRange inputs,
+ Value buildUnresolvedArgumentMaterialization(Block *block, Location loc,
+ ValueRange inputs,
Type origOutputType,
Type outputType,
const TypeConverter *converter);
@@ -903,8 +902,6 @@ struct ConversionPatternRewriterImpl : public RewriterBase::Listener {
// State
//===--------------------------------------------------------------------===//
- PatternRewriter &rewriter;
-
/// This rewriter must be used for erasing ops/blocks.
SingleEraseRewriter eraseRewriter;
@@ -1008,8 +1005,12 @@ void BlockTypeConversionRewrite::rollback() {
LogicalResult BlockTypeConversionRewrite::materializeLiveConversions(
function_ref<Operation *(Value)> findLiveUser) {
+ auto builder = OpBuilder::atBlockBegin(block, /*listener=*/&rewriterImpl);
+
// Process the remapping for each of the original arguments.
for (unsigned i = 0, e = origBlock->getNumArguments(); i != e; ++i) {
+ OpBuilder::InsertionGuard g(builder);
+
// If the type of this argument changed and the argument is still live, we
// need to materialize a conversion.
BlockArgument origArg = origBlock->getArgument(i);
@@ -1021,14 +1022,12 @@ LogicalResult BlockTypeConversionRewrite::materializeLiveConversions(
Value replacementValue = rewriterImpl.mapping.lookupOrDefault(origArg);
bool isDroppedArg = replacementValue == origArg;
- if (isDroppedArg)
- rewriterImpl.rewriter.setInsertionPointToStart(getBlock());
- else
- rewriterImpl.rewriter.setInsertionPointAfterValue(replacementValue);
+ if (!isDroppedArg)
+ builder.setInsertionPointAfterValue(replacementValue);
Value newArg;
if (converter) {
newArg = converter->materializeSourceConversion(
- rewriterImpl.rewriter, origArg.getLoc(), origArg.getType(),
+ builder, origArg.getLoc(), origArg.getType(),
isDroppedArg ? ValueRange() : ValueRange(replacementValue));
assert((!newArg || newArg.getType() == origArg.getType()) &&
"materialization hook did not provide a value of the expected "
@@ -1293,6 +1292,8 @@ LogicalResult ConversionPatternRewriterImpl::convertNonEntryRegionTypes(
Block *ConversionPatternRewriterImpl::applySignatureConversion(
Block *block, const TypeConverter *converter,
TypeConverter::SignatureConversion &signatureConversion) {
+ MLIRContext *ctx = block->getParentOp()->getContext();
+
// If no arguments are being changed or added, there is nothing to do.
unsigned origArgCount = block->getNumArguments();
auto convertedTypes = signatureConversion.getConvertedTypes();
@@ -1309,7 +1310,7 @@ Block *ConversionPatternRewriterImpl::applySignatureConversion(
// Map all new arguments to the location of the argument they originate from.
SmallVector<Location> newLocs(convertedTypes.size(),
- rewriter.getUnknownLoc());
+ Builder(ctx).getUnknownLoc());
for (unsigned i = 0; i < origArgCount; ++i) {
auto inputMap = signatureConversion.getInputMapping(i);
if (!inputMap || inputMap->replacementValue)
@@ -1328,8 +1329,6 @@ Block *ConversionPatternRewriterImpl::applySignatureConversion(
SmallVector<std::optional<ConvertedArgInfo>, 1> argInfo;
argInfo.resize(origArgCount);
- OpBuilder::InsertionGuard guard(rewriter);
- rewriter.setInsertionPointToStart(newBlock);
for (unsigned i = 0; i != origArgCount; ++i) {
auto inputMap = signatureConversion.getInputMapping(i);
if (!inputMap)
@@ -1372,7 +1371,7 @@ Block *ConversionPatternRewriterImpl::applySignatureConversion(
outputType = legalOutputType;
newArg = buildUnresolvedArgumentMaterialization(
- rewriter, origArg.getLoc(), replArgs, origOutputType, outputType,
+ newBlock, origArg.getLoc(), replArgs, origOutputType, outputType,
converter);
}
@@ -1410,12 +1409,11 @@ Value ConversionPatternRewriterImpl::buildUnresolvedMaterialization(
return convertOp.getResult(0);
}
Value ConversionPatternRewriterImpl::buildUnresolvedArgumentMaterialization(
- PatternRewriter &rewriter, Location loc, ValueRange inputs,
- Type origOutputType, Type outputType, const TypeConverter *converter) {
- return buildUnresolvedMaterialization(
- MaterializationKind::Argument, rewriter.getInsertionBlock(),
- rewriter.getInsertionPoint(), loc, inputs, outputType, origOutputType,
- converter);
+ Block *block, Location loc, ValueRange inputs, Type origOutputType,
+ Type outputType, const TypeConverter *converter) {
+ return buildUnresolvedMaterialization(MaterializationKind::Argument, block,
+ block->begin(), loc, inputs, outputType,
+ origOutputType, converter);
}
Value ConversionPatternRewriterImpl::buildUnresolvedTargetMaterialization(
Location loc, Value input, Type outputType,
@@ -1527,7 +1525,7 @@ void ConversionPatternRewriterImpl::notifyMatchFailure(
ConversionPatternRewriter::ConversionPatternRewriter(
MLIRContext *ctx, const ConversionConfig &config)
: PatternRewriter(ctx),
- impl(new detail::ConversionPatternRewriterImpl(*this, config)) {
+ impl(new detail::ConversionPatternRewriterImpl(ctx, config)) {
setListener(impl.get());
}
|
jpienaar
approved these changes
Feb 20, 2024
577b5eb
to
58e2c18
Compare
Base automatically changed from
users/matthias-springer/conversion_config
to
main
February 23, 2024 10:28
… `ConversionPatternRewriter` `ConversionPatternRewriterImpl` no longer maintains a reference to the respective `ConversionPatternRewriter`. An `MLIRContext` is sufficient. This commit simplifies the internal state of `ConversionPatternRewriterImpl`.
362813d
to
70920e8
Compare
matthias-springer
added a commit
to matthias-springer/llvm-project
that referenced
this pull request
Feb 23, 2024
This is a follow-up to llvm#82333. It possible that the target block of a `BlockTypeConversionRewrite` is detached, so the `MLIRContext` cannot be taken from the block.
matthias-springer
added a commit
to matthias-springer/llvm-project
that referenced
this pull request
Feb 23, 2024
This is a follow-up to llvm#82333. It possible that the target block of a `BlockTypeConversionRewrite` is detached, so the `MLIRContext` cannot be taken from the block.
matthias-springer
added a commit
that referenced
this pull request
Feb 23, 2024
This is a follow-up to #82333. It is possible that the target block of a `BlockTypeConversionRewrite` is detached, so the `MLIRContext` cannot be taken from the block.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ConversionPatternRewriterImpl
no longer maintains a reference to the respectiveConversionPatternRewriter
. AnMLIRContext
is sufficient. This commit simplifies the internal state ofConversionPatternRewriterImpl
.