Hello!
In our project we faced with some weird behavior of partial dialect conversion.
I’d like to know is it a bug or do we misunderstand its logic.
We have a ConversionTarget which explicitly marks OpA as illegal and OpB as legal and a RewriterPattern which converts OpA to OpB. But during conversion it creates third extra operation (std.constant for example). If we don’t mark this operation as explicitly legal in the ConversionTarget the driver fails to apply the pattern. But applyPartialConversion call returned success status despite the fact that it didn’t convert explicitly illegal operation OpA.
That was definitely a bug in our code (forgot to mark extra operation as legal), but we expected that applyPartialConversion will fail if it wasn’t able to convert operation explicitly marked as illegal.
The documentation of the applyPartialConversion function says:
A partial conversion will legalize as many operations to the target as possible, but will allow pre-existing operations that were not explicitly marked as “illegal” to remain unconverted. This allows for partially lowering parts of the input in the presence of unknown operations.
So, do we faced with a bug in the applyPartialConversion (it returns success even if it failed to convert explicitly illegal operation) or we have some misunderstanding of its behavior?
That feels like a bug. That should be handled by: https://github.com/llvm/llvm-project/blob/be6c49e743d5ceaced202d29d51d94b2d210240a/mlir/lib/Transforms/Utils/DialectConversion.cpp#L2225
How were you setting the op as illegal? If you can craft an upstream repro, I can take a look.
– River
I was able to create a small reproducer test. The issue is that it is use addDynamicallyLegalOp method to mark the operation as illegal.
// This works for both Full and Partial mode
target.addIllegalOp<ILLegalOpG>();
target.addLegalOp<LegalOpC>();
// convertes ILLegalOpG to LegalOpC, but creates unregistered operation
patterns.add<TestCreateUnregisteredOp>(&getContext());
assert(failed(applyPartialConversion(getOperation(), target, std::move(patterns)));
assert(failed(applyFullConversion(getOperation(), target, std::move(patterns)));
// This works for Full conversion, but not for Partial mode
target.addDynamicallyLegalOp<ILLegalOpG>([](ILLegalOpG) { return false; });
target.addLegalOp<LegalOpC>();
// convertes ILLegalOpG to LegalOpC, but creates unregistered operation
patterns.add<TestCreateUnregisteredOp>(&getContext());
// BUG: assert(failed(applyPartialConversion(getOperation(), target, std::move(patterns)));
assert(failed(applyFullConversion(getOperation(), target, std::move(patterns)));
Instead of reporting an error, the applyPartialConversion just add the ILLegalOpG to unconvertedOps.
I assume this is a bug, since addDynamicallyLegalOp makes an operation known to the converter, so it can check its legality.
Right that makes sense. Originally, being dynamically legal didn’t count as “explicitly illegal”. The explicit was markIllegal(or however that is spelled). I think it makes sense to expand the definition to include the dynamic legality nowadays.
– River