Skip to content

[mlir][IR] Make RewriterBase::replaceOp non-virtual #160529

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[mlir][IR] Make RewriterBase::replaceOp non-virtual #160529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions mlir/include/mlir/IR/PatternMatch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,12 +515,12 @@ class RewriterBase : public OpBuilder {
/// Replace the results of the given (original) operation with the specified
/// list of values (replacements). The result types of the given op and the
/// replacements must match. The original op is erased.
virtual void replaceOp(Operation *op, ValueRange newValues);
void replaceOp(Operation *op, ValueRange newValues);

/// Replace the results of the given (original) operation with the specified
/// new op (replacement). The result types of the two ops must match. The
/// original op is erased.
virtual void replaceOp(Operation *op, Operation *newOp);
void replaceOp(Operation *op, Operation *newOp);

/// Replace the results of the given (original) op with a new op that is
/// created without verification (replacement). The result values of the two
Expand Down
16 changes: 1 addition & 15 deletions mlir/include/mlir/Transforms/DialectConversion.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,7 +900,7 @@ class ConversionPatternRewriter final : public PatternRewriter {
/// RewriterBase APIs, (3) may be removed in the future.
void replaceAllUsesWith(Value from, ValueRange to);
void replaceAllUsesWith(Value from, Value to) override {
replaceAllUsesWith(from, ValueRange{to});
replaceAllUsesWith(from, to ? ValueRange{to} : ValueRange{});
Copy link
Member

Choose a reason for hiding this comment

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

I see tests failing when removing this change and I am slightly confused why it is suddenly needed.
Should we at least put a TODO here that this should be removed in the future and fix the users instead? IIRC this does not work in the greedy pattern rewriter and I think its best we keep the differences to a minimum

Copy link
Member Author

@matthias-springer matthias-springer Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This function is the 1:1 version of replaceAllUsesWith. It dispatches to the 1:N version. Replacing with a "null" value means dropping all uses of the value. You cannot do that with a regular rewriter, but the dialect conversion API allows it and existing code relies on it. (We may want to change this at some point.) When you drop a value that still has uses, an out-of-thin-air source materialization is inserted.

The problem with "null" is that we would insert "null" values into the ConversionValueMapping. This "null" check that you're seeing here used to be part of ConversionPatternRewriter::replaceOp in the original code.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds to me like it effectively attempts to be a 1:0 dialect conversion and something worth adapting in the future

}

/// Return the converted value of 'key' with a type defined by the type
Expand All @@ -923,20 +923,6 @@ class ConversionPatternRewriter final : public PatternRewriter {
/// patterns even if a failure is encountered during the rewrite step.
bool canRecoverFromRewriteFailure() const override { return true; }

/// Replace the given operation with the new values. The number of op results
/// and replacement values must match. The types may differ: the dialect
/// conversion driver will reconcile any surviving type mismatches at the end
/// of the conversion process with source materializations. The given
/// operation is erased.
void replaceOp(Operation *op, ValueRange newValues) override;

/// Replace the given operation with the results of the new op. The number of
/// op results must match. The types may differ: the dialect conversion
/// driver will reconcile any surviving type mismatches at the end of the
/// conversion process with source materializations. The original operation
/// is erased.
void replaceOp(Operation *op, Operation *newOp) override;

/// Replace the given operation with the new value ranges. The number of op
/// results and value ranges must match. The given operation is erased.
void replaceOpWithMultiple(Operation *op,
Expand Down
Loading