[Tosa][tosa-to-linalg] Code design problem of Tosa to Linalg conversion

When I use mlir-opt -pass-pipeline="func.func(tosa-to-linalg)" to downgrade the tosa dialect,
I encounter the error failed to legalize operation 'tosa.max_pool2d'.
After reading the source code, I find that this pass can only handle part ops of Tosa,
while some of the rest ops are marked as legal directly in TosaToLinalgPass.cpp .

    // in file TosaToLinalgPass.cpp

    target.addLegalOp<tosa::ApplyScaleOp>();
    target.addLegalOp<tosa::IfOp>();
    target.addLegalOp<tosa::ConstOp>();
    target.addLegalOp<tosa::WhileOp>();
    target.addLegalOp<tosa::SliceOp>();

but the other part is not processed, such as tosa.max_pool2d,
I don’t understand why we don’t mark the remaining ops(like tosa.depthwise_conv2d,tosa.avg_pool2d,etc) as legal?

I also found that these ops(cannot be processed by tosa-to-linalg) can be degraded through tosa-to-linalg-named.
Does this mean that I must call tosa-to-linalg-named by default before using tosa-to-linalg?

A nit: we use the term “lower” instead of “downgrade” in general, or “convert” eventually.

For the rest of your question, I’ll leave it to someone more knowledgeable about these passes, but your observations seem very relevant to me: TosaToLinalgPass.cpp should probably use a partial conversion or be absolutely complete in terms of defining the legality criteria.

As for the list of passes to perform the conversion, see this function: llvm-project/TosaToLinalgPass.cpp at main · llvm/llvm-project · GitHub
(this should really be registered as a pipeline I think…)

Thank you very much for your reply. Using the pass pipe you mentioned can really achieve the lowering without errors,
but we want to build the pipeline ourselves and add some additional operator fusion work.
Mlir is a very flexible framework, after each pass processes the operator it is responsible for,
it should not affect the subsequent pass’s processing of other operators.
As you mentioned, it would make sense for us if you could fix the code here, either by marking operators as legal or using a partial conversion.
I raised an issue here https://github.com/llvm/llvm-project/issues/59225, looking forward to your update!

I’m trying to decide whether that pass is better thought of as more of a test pass for a canonical lowering flow or a building block intended to construct larger things out of. It reads to me a bit more like the former, but this is just an impression formed from a 60s read.

@rsuderman, @jpienaar any opinion?

Marking those ops as legal there feels like simulating a partial conversion while using a full conversion (so I think it needs a bit of cleanup). That lowering to Linalg only supports some ops but the pass seems to be doing what it says with the exception that it expects a specific starting state.

@JiaweiRen in your use case would a partial conversion where unhandled ops are just left as is work?

I think Stella is right that we should probably move away from thinking of tosa-to-linalg as a pass and instead expose the rewriters for custom passes. We have tosa-to-linalg, tosa-to-linalg-named, and tosa-to-scf all running as separate passes but there is not go reason fro this.

@jpienaar what are your thoughts on us migrating to rewriter sets with the passes focusing on just testing? We could maintain the existing passes but emaphasize these are primarily for testing purposes.

Yes, partial conversion can also work. :handshake: