[mlir][Transforms] Dialect conversion: Skip materializations when running without converter #101318
Conversation
…ning without converter
|
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-mlir Author: Matthias Springer (matthias-springer) ChangesExperiment only. Do not merge. TODO: test case Full diff: https://github.com/llvm/llvm-project/pull/101318.diff 2 Files Affected:
diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index f26aa0a1516a6..fdd0175ffae53 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -1316,37 +1316,43 @@ Block *ConversionPatternRewriterImpl::applySignatureConversion(
continue;
}
- // This is a 1->1+ mapping. 1->N mappings are not fully supported in the
- // dialect conversion. Therefore, we need an argument materialization to
- // turn the replacement block arguments into a single SSA value that can be
- // used as a replacement.
+ // This is a 1->1+ mapping.
auto replArgs =
newBlock->getArguments().slice(inputMap->inputNo, inputMap->size);
+
+ // When there is no type converter, assume that the new block argument
+ // types are legal. This is reasonable to assume because they were
+ // specified by the user.
+ // FIXME: This won't work for 1->N conversions because multiple output
+ // types are not supported in parts of the dialect conversion. In such a
+ // case, we currently use the original block argument type (produced by
+ // the argument materialization).
+ if (!converter && replArgs.size() == 1) {
+ mapping.map(origArg, replArgs[0]);
+ appendRewrite<ReplaceBlockArgRewrite>(block, origArg);
+ continue;
+ }
+
+ // 1->N mappings are not fully supported in the dialect conversion.
+ // Therefore, we need an argument materialization to turn the replacement
+ // block arguments into a single SSA value (of the original type) that can
+ // be used as a replacement.
Value argMat = buildUnresolvedMaterialization(
MaterializationKind::Argument, newBlock, newBlock->begin(),
origArg.getLoc(), /*inputs=*/replArgs, origArgType, converter);
mapping.map(origArg, argMat);
appendRewrite<ReplaceBlockArgRewrite>(block, origArg);
+ // Now legalize the type by building a target materialization.
Type legalOutputType;
- if (converter) {
+ if (converter)
legalOutputType = converter->convertType(origArgType);
- } else if (replArgs.size() == 1) {
- // When there is no type converter, assume that the new block argument
- // types are legal. This is reasonable to assume because they were
- // specified by the user.
- // FIXME: This won't work for 1->N conversions because multiple output
- // types are not supported in parts of the dialect conversion. In such a
- // case, we currently use the original block argument type (produced by
- // the argument materialization).
- legalOutputType = replArgs[0].getType();
- }
if (legalOutputType && legalOutputType != origArgType) {
Value targetMat = buildUnresolvedTargetMaterialization(
origArg.getLoc(), argMat, legalOutputType, converter);
mapping.map(argMat, targetMat);
+ appendRewrite<ReplaceBlockArgRewrite>(block, origArg);
}
- appendRewrite<ReplaceBlockArgRewrite>(block, origArg);
}
appendRewrite<BlockTypeConversionRewrite>(newBlock, block, converter);
diff --git a/mlir/test/Transforms/test-legalize-type-conversion.mlir b/mlir/test/Transforms/test-legalize-type-conversion.mlir
index d0563fed8e5d9..07dfb49473f5e 100644
--- a/mlir/test/Transforms/test-legalize-type-conversion.mlir
+++ b/mlir/test/Transforms/test-legalize-type-conversion.mlir
@@ -103,8 +103,9 @@ func.func @test_block_argument_not_converted() {
// Make sure argument type changes aren't implicitly forwarded.
func.func @test_signature_conversion_no_converter() {
"test.signature_conversion_no_converter"() ({
- // expected-error@below {{failed to legalize unresolved materialization from ('f64') to 'f32' that remained live after conversion}}
+ // expected-error@below {{failed to materialize conversion for block argument #0 that remained live after conversion, type was 'f32'}}
^bb0(%arg0: f32):
+ // expected-note@below{{see existing live user here}}
"test.type_consumer"(%arg0) : (f32) -> ()
"test.return"(%arg0) : (f32) -> ()
}) : () -> ()
|
You can test this locally with the following command:git-clang-format --diff 5c406eacf4f4dda0cf9267d638954aa20f17e118 59448a2a65a950da3be20c0b495cec59246c66b6 --extensions cpp -- mlir/lib/Transforms/Utils/DialectConversion.cppView the diff from clang-format here.diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index fdd0175ffa..b9a197e89e 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -1319,7 +1319,7 @@ Block *ConversionPatternRewriterImpl::applySignatureConversion(
// This is a 1->1+ mapping.
auto replArgs =
newBlock->getArguments().slice(inputMap->inputNo, inputMap->size);
-
+
// When there is no type converter, assume that the new block argument
// types are legal. This is reasonable to assume because they were
// specified by the user.
|
|
Merging this to address the immediate failure as mentioned at #101148 (comment) :) |
|
@MaskRay : the PR is described as:
I don't get why it would be OK to merge here, especially without test case. I also don't understand the urgency: unless there is an upstream bot broken, it's doesn't seem like an OK thing to merge this! Downstream specific issues should be handled downstream (you can apply this patch in your fork downstream, you can revert the original patch, you can disable your failing tests, you can wait a few days for this patch to merge upstream, etc.), there is no reason to mess-up with upstream. It wasn't even passing the code formatting! |
|
I agree, this should not have been merged without a test. And by now I believe that in fact there is something wrong with the usage of DialectConversion in the case of the failures I looked at so far. It seems that recently a workaround was added to skip UnrealizedConversionCast operands in the conversion patterns. If I remove these workarounds, the tests pass. My guess is that #101148 was in fact fixing the issue that required the usage of the workarounds before. |
|
Thanks for the for the follow-up @akuegel ! |
You are right, it was not OK to merge here. Apologies, the situation wasn't clear to me and as I do not normally contribute to MLIR, I made a bad judgement here. I did perform some downstream tests and verified that the workaround (101148) would work. More thorough investigation was necessary before reverting or merging and I should have exercised greater caution. Sorry for jumping the gun. edit: I guess the culprit was a previous workaround: tensorflow/tensorflow@be9752e#diff-519e282d6e69fe87c47fd37c519bf65df2ed9f97490b427dc7961af92d888c92 |
|
No worry, that happens! Thanks. |
Experiment only. Do not merge.
TODO: test case