Skip to content

Commit

Permalink
[mlir][Transforms][NFC] Simplify buildUnresolvedMaterialization imp…
Browse files Browse the repository at this point in the history
…lementation (llvm#121651)

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.

The previous implementation seemed incorrect because
`buildUnresolvedMaterialization` created 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 `findOrBuildReplacementValue` that was
incorrect.
  • Loading branch information
matthias-springer authored Jan 5, 2025
1 parent 8b57704 commit 486f83f
Showing 1 changed file with 5 additions and 12 deletions.
17 changes: 5 additions & 12 deletions mlir/lib/Transforms/Utils/DialectConversion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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();
Expand Down Expand Up @@ -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(),
Expand Down

0 comments on commit 486f83f

Please sign in to comment.