[mlir][Transforms][NFC] Simplify buildUnresolvedMaterialization implementation
#121651
Conversation
This file contains hidden or 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
…lementation The `buildUnresolvedMaterialization` implementation used to check if a materialization is necessary. A materialization is not necessary if the desired types already match the input. However, this situation can never happen: we look for mapped values with the desired type at the call sites before requesting a new unresolved materialization.
Member
|
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-mlir Author: Matthias Springer (matthias-springer) ChangesThe The previous implementation seemed incorrect because Also fix a comment that in Full diff: https://github.com/llvm/llvm-project/pull/121651.diff 1 Files Affected:
diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 0e577d2d39de3d..fbecb874f17e0a 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -1430,13 +1430,8 @@ ValueRange ConversionPatternRewriterImpl::buildUnresolvedMaterialization(
UnrealizedConversionCastOp *castOp) {
assert((!originalType || kind == MaterializationKind::Target) &&
"original type is valid only for target materializations");
-
- // Avoid materializing an unnecessary cast.
- if (TypeRange(inputs) == outputTypes) {
- if (!valuesToMap.empty())
- mapping.map(std::move(valuesToMap), inputs);
- return inputs;
- }
+ assert(TypeRange(inputs) != outputTypes &&
+ "materialization is not necessary");
// Create an unresolved materialization. We use a new OpBuilder to avoid
// tracking the materialization like we do for other operations.
@@ -1455,7 +1450,9 @@ ValueRange ConversionPatternRewriterImpl::buildUnresolvedMaterialization(
Value ConversionPatternRewriterImpl::findOrBuildReplacementValue(
Value value, const TypeConverter *converter) {
- // Find a replacement value with the same type.
+ // Try to find a replacement value with the same type in the conversion value
+ // mapping. This includes cached materializations. We try to reuse those
+ // instead of generating duplicate IR.
ValueVector repl = mapping.lookupOrNull(value, value.getType());
if (!repl.empty())
return repl.front();
@@ -1489,10 +1486,6 @@ Value ConversionPatternRewriterImpl::findOrBuildReplacementValue(
// in the conversion value mapping.) The insertion point of the
// materialization must be valid for all future users that may be created
// later in the conversion process.
- //
- // Note: Instead of creating new IR, `buildUnresolvedMaterialization` may
- // return an already existing, cached materialization from the conversion
- // value mapping.
Value castValue =
buildUnresolvedMaterialization(MaterializationKind::Source,
computeInsertPoint(repl), value.getLoc(),
|
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.
The
buildUnresolvedMaterializationimplementation used to check if a materialization is necessary. A materialization is not necessary if the desired types already match the input. However, this situation can never happen: we look for mapped values with the desired type at the call sites before requesting a new unresolved materialization.The previous implementation seemed incorrect because
buildUnresolvedMaterializationcreated a mapping that is never rolled back. (When in reality that code was never executed, so it is technically not incorrect.)Also fix a comment that in
findOrBuildReplacementValuethat was incorrect.