Skip to content

[mlir][Transforms] Fix compile time regression in dialect conversion #83023

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

Merged
merged 1 commit into from
Feb 26, 2024
Merged

[mlir][Transforms] Fix compile time regression in dialect conversion #83023

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
13 changes: 4 additions & 9 deletions mlir/lib/Transforms/Utils/DialectConversion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1229,9 +1229,8 @@ LogicalResult ConversionPatternRewriterImpl::remapValues(
}

bool ConversionPatternRewriterImpl::isOpIgnored(Operation *op) const {
// Check to see if this operation was replaced or its parent ignored.
return ignoredOps.count(op->getParentOp()) ||
hasRewrite<ReplaceOperationRewrite>(rewrites, op);
// Check to see if this operation or the parent operation is ignored.
return ignoredOps.count(op->getParentOp()) || ignoredOps.count(op);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the parentOp more likely to be ignored? Otherwise why not check the op first before dereferencing the 2 pointers to get the parent?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to partially undo this change (#83051) because of downstream code that uses the rewriter API incorrectly: accessing nested regions of an op after replacing the op; the dialect conversion does not crash when doing that because ops are not really erased the very end. But this change caused those nested ops to be ignored. I'm going to fix that code, then I should be able to change this to simply ignoredOps.count(op).

}

void ConversionPatternRewriterImpl::markNestedOpsIgnored(Operation *op) {
Expand Down Expand Up @@ -1480,12 +1479,7 @@ void ConversionPatternRewriterImpl::notifyOperationInserted(
void ConversionPatternRewriterImpl::notifyOpReplaced(Operation *op,
ValueRange newValues) {
assert(newValues.size() == op->getNumResults());
#ifndef NDEBUG
for (auto &rewrite : rewrites)
if (auto *opReplacement = dyn_cast<ReplaceOperationRewrite>(rewrite.get()))
assert(opReplacement->getOperation() != op &&
"operation was already replaced");
#endif // NDEBUG
assert(!ignoredOps.contains(op) && "operation was already replaced");

// Track if any of the results changed, e.g. erased and replaced with null.
bool resultChanged = false;
Expand All @@ -1506,6 +1500,7 @@ void ConversionPatternRewriterImpl::notifyOpReplaced(Operation *op,

// Mark this operation as recursively ignored so that we don't need to
// convert any nested operations.
ignoredOps.insert(op);
markNestedOpsIgnored(op);
}

Expand Down