[mlir][test] Test conversion of TOSA to EmitC via LinAlg

[ilovepi]

This is an important use case for MLGO, to simplify maintenance and
foster easier to share models via C headers.

[ilovepi]

@mtrofin @simon-camp, sorry that this is about a week behind schedule. I had most of this done last week, but I was trying to figure out why some of the transforms weren't working as expected. I believe that using the tosa-to-linalg.mlir test as a basis may have been a mistake.

Once we settle on the precise input MLIR, alls that's left to do is to validate the generated header via CHECK lines.

@simon-camp perhaps you have an idea why the second RUN line is failing? These are all basically translations from your script, and there shouldn't be any material deviations from the original.

# .---command stderr------------
# | /usr/local/google/home/paulkirth/llvm-fork/build/tools/mlir/test/Conversion/TosaToEmitC/Output/tosa-to-linalg.mlir.tmp.model.linalg.mlir:19:1: error: redefinition of attribute alias id 'map'
# | #map = affine_map<(d0) -> (d0)>
# | ^
# `-----------------------------
# error: command failed with exit status: 1

[simon-camp]

What's happening is that the --split-input-file argument splices the file on the // ----- lines, runs the passes on each chunk independently and joins the results back together. So multiple chunks generate this attribute independently with the same name. This then fails to parse in the next run line.

TL;DR: remove the --split-input-filearguments.

Interestingly the test then runs to line 15 where it crashes. The input to that invocation is this. I expected the lowering to fail there as we support neither dynamic shapes nor 0d memrefs in the conversion to EmitC, but the passes should fail with an error message instead of an assertion. I can take a look at this next week.

[ilovepi]

What's happening is that the --split-input-file argument splices the file on the // ----- lines, runs the passes on each chunk independently and joins the results back together. So multiple chunks generate this attribute independently with the same name. This then fails to parse in the next run line.

TL;DR: remove the --split-input-filearguments.

Thanks. I'll update the patch in a bit.

Interestingly the test then runs to line 15 where it crashes. The input to that invocation is this. I expected the lowering to fail there as we support neither dynamic shapes nor 0d memrefs in the conversion to EmitC, but the passes should fail with an error message instead of an assertion. I can take a look at this next week.

I guess that may require some of the post processing you mentioned? I'll see if I can add some of those python clean ups you provided in the meantime. We can add some TODO's and remove those as the functionality improves.

[simon-camp]

I pushed a fix for the crash in #94936. I would suggest that you test the script on an actual model, as it should have fixed shapes. That will not run into most of unsupported features seen in the current tests. Feel free to share the input model in the TOSA dialect if things don't work directly.

[ilovepi]

I pushed a fix for the crash in #94936. I would suggest that you test the script on an actual model, as it should have fixed shapes. That will not run into most of unsupported features seen in the current tests. Feel free to share the input model in the TOSA dialect if things don't work directly.

Thanks for the suggestion. Since this was testing the conversion, from ToSa -> LinAlg -> EmitC, I assumed using the tests for ToSa -> LinAlg would be a good starting point.

I'm not all that familiar w/ how the models are derived. Do you have a suggestion as to how we should generate a minimal case? On the LLVM side, I'd either start w/ some minimal C code and generate IR or write it by hand. I'm a little out of my depth on the MLIR side of things, and its never been clear to me how the MLIR used for MLGO is derived in the first place.

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[ilovepi]

@simon-camp I finally made some progress here after getting some help from @mtrofin on the saved model. For now I've gone w/ a super simple model from TF Lite and we can use more complex ones once we fix the issues w/ the test/conversion pipeline.

Right now the main blocker I have is that the final --convert-memref-to-emitc is failing to when trying to convert the function argument. Looking at the failure and some of the tests around memref conversion, this seems intentional, as I found tests that are specifically checking that this conversion fails. I would have supposed that this could be transformed into an emitc array type or pointer, but it seems not to be supported.

I think this is happening because we don't have a materialization callback available in TypeConverter::convertSignatureArgs(), but it is not clear to me if we should add something there, add some other transform in the placeholder python script, or take a completely different approach.

Error:

<stdin>:26:5: error: failed to legalize unresolved materialization from ('memref<1xf32>') to '!emitc.array<1xf32>' that remained live after conversion
    memref.store %20, %arg2[%2] : memref<1xf32>
    ^
<stdin>:26:5: note: see current operation: %0 = "builtin.unrealized_conversion_cast"(%arg2) : (memref<1xf32>) -> !emitc.array<1xf32>
<stdin>:26:5: note: see existing live user here: %29 = emitc.subscript %0[%5] : (!emitc.array<1xf32>, index) -> !emitc.lvalue<f32>

[simon-camp]

Hi @ilovepi, @mtrofin. I had some time to look into the failure. I have an idea how to fix the error; I'm just waiting on feedback from other folks before sending a PR. Alongside that I stripped down the pass pipeline to a minimal version working for the new test case.

I have a working prototype here that get's completely rid of the Python hack.

[ilovepi]

@simon-camp, that's great news! The prototype looks much nicer than using the complicated nest of RUN lines. Is there a PR for the prototype yet? if so, we can probably abandon this, since your implementation looks much better, and we can just keep incrementally adding more test cases after that.

[ilovepi]

@simon-camp I saw #114204 landed. What's the delta between upstream and your prototype? It seems like there's a bit more of the conversion changes to land. Are these things we can help with?

[simon-camp]

Great timing @ilovepi, I wanted to post an update today before I go on vacation.

There are 2-3 PRs missing:

• (NFC) Moving the memref type conversion to a shared location
  • move populateMemRefToEmitCTypeConversion to Dialect/EmitC/Transforms/TypeConversions as is
• Adapt the SCF to EmitC conversion to use the memref type conversion
• (Add support to promote 0d memrefs to 1d arrays in the type converter and conversion)
  • This is not strictly necessary to get something going end2end, we can change your TOSA testcase to work on 1d tensors

I will be back at work in mid-November and can continue working on it.

[Jerry-Ge]

Hi @ilovepi , are you still working on this PR? Should we close it?

[ilovepi]

Ah, right, @simon-camp's conversion work in #117549 supersedes this work. Thanks for the reminder to close.