[AOT] Name mangling in AOT by giuseros · Pull Request #8014 · apache/tvm

giuseros · 2021-05-10T23:09:45Z

Mini-RFC is here: https://discuss.tvm.apache.org/t/mini-rfc-name-mangling-in-aot

With this change we'll mangle the name of global symbols so that we can bundle
together multiple models in the same application.

The relay.build interface has been left unchanged, which means I am
resuing mod_name as a prefix for all functions.

If mod_name is None then tvmgen_ prefix is used
If mod_name is specified then tvmgen_mod_name_ prefix is used

I refactored the aot test utils and added some tests for multiple
models.

Change-Id: I310af75c24e422861aeaceb3c3cd4cd602071df5

giuseros · 2021-05-10T23:10:44Z

cc: @manupa-arm @areusch @Mousius

giuseros · 2021-05-11T13:30:56Z

I marked the naming comments "resolved". The meaning is that we will discuss those on the RFC here: https://discuss.tvm.apache.org/t/mini-rfc-name-mangling-in-aot/

giuseros · 2021-05-12T11:17:30Z

I made the name-mangling logic completely decoupled by the non-name-mangling logic. This means that name mangling is enabled only in AOT and in PartitionGraphWithModName, the rest of TVM won't be using this feature by default.

areusch

hi @giuseros, thanks for adding this! here's an initial round of review comments

manupak

Hi @giuseros ,

Sorry been busy a bit :)

I did an initial pass.

I guess my main concern is why are we duplicating lowering functions with and without name mangling support. I would naturally think user would want to name mangle the function names if a mod_name is provided and mangled names are strictly superior to non-mangled polluted names, irrespective of whether they are being used in AoT or not, unless Im missing something.

I'd like to hear @areusch thoughts as well.

giuseros · 2021-05-24T13:13:56Z

Hi @manupa-arm ,
Thanks for your reply. I pushed a new set of changes trying to address your comments:

Now I am using name-mangling as default in all TVM, removing duplication for non-mangled behaviour.
The MangleGraph pass is now the default in the partitioning pipeline
The mangled mod-name is formed in python and passed down in C++ where we only concatenate to the function names

I guess that, in addition to make the tests pass, we need to agree on naming:

MangleGraph. Is MangleExternalFunction better? Because I am only mangling the functions that will be offloaded to the external compiler
run_func: is it ok for everyone if I use only run? My problem with only run is that it seems a bit too generic. Maybe run_model? What do you think?

giuseros · 2021-06-03T17:41:27Z

Hi @manupa-arm , @areusch ,
All the tests have finally passed. Please, let me know if there is anything else I should address.

areusch

just one other nit (honestly mostly care about the unit test, but adding a comment since i saw this)

@giuseros

This introduces structures generated to provide a documented and stable user friendly interface to a TVM generated model, as can be seen in the AOT demo application: ``` struct tvm_default_inputs inputs = { .input_1 = input_data, }; struct tvm_default_outputs outputs = { .output = output_data, }; int ret_val = tvm_default_run(&inputs, &outputs, NULL, NULL); ``` To facilitate this, some other changes are included: * Removed dependency on `aot_executor.{c,h}` in tests, pending the discussion in the interface RFC as to whether we keep them. * Moved creation of test DLTensor's into the AOT test utils, in future this can be replaced by loading via the Python API or otherwise * Introduce `parametrize_aot_options` which can be used to test permutations of AOT which work together - for now this filters C interface and packed operators * Updated demo application to generate the header for demonstration purposes, we should consider porting the demo application to Model Library Format and using the toolchain in the Zephyr App via CMake instead? This patch builds upon the improvements @giuseros made to AOT testing and will greatly benefit from name mangling from apache#8014

areusch · 2021-06-22T00:39:35Z

apologies, i think i missed this and it now needs a rebase or merge.

i discussed this with @jroesch a bit as well, who is now working in earnest to merge #7518 in the next two weeks. following that PR, we'll arrive at a state where we can begin to think of the entire compiler as a series of IRModule passes. our discussion was that, at that point, it probably makes sense to move things like name mangling and e.g. the type signature changes proposed by @Mousius to one end of the compiler stack (i.e. at the relay level or at the end of the TIR passes). this way, the middle part of the compiler remains as generic as possible.

we'd like to propose the following:

let us sync and merge this PR
we'll land [RFC]TECompiler: Staged refactor and removal of compile engine #7518 on top of this in the next two weeks (e.g. aiming for before July 4 US holiday)
following that PR, we'll likely reorganize this and move it to the end of the TIR passes

to that end, we'd like to briefly prioritize #7518 next week over AOT changes to reduce the churn and let it land, since it's a fairly invasive PR.

if that's ok with you guys, i'll hand this off to @jroesch to merge while i'm ooo next few days.

Mini-RFC is here: https://discuss.tvm.apache.org/t/mini-rfc-name-mangling-in-aot With this change we'll mangle the name of global symbols so that we can bundle together multiple models in the same application. The relay.build interface has been left unchanged, which means I am resuing mod_name as a prefix for all functions. If mod_name is None then a "_tvm" prefix is used. I had to add two different compilation functions: - _CompileEngineLowerWithModuleName to mangle all the operators with the mod_name - PartitionGraphWithModName to mangle all the operators produced by BYOC I could have changed signature of both, but that would have meant a very invasive refactoring. I refactored the aot test utils and added some tests for multiple models. Change-Id: I30e93fa075f660054577ea36cf9268ec0c6eebcb

giuseros · 2021-06-22T14:30:26Z

Hi @areusch , @jroesch ,
I would say, let's get this merged and once #7518 lands, we can sync-up if we want to have the mangling as a pass or as a feature into the compiler. I wouldn't have any strong opinions (and honestly I like the idea of having it as a pass) but maybe @manupa-arm or @Mousius might.

Thanks,
Giuseppe

giuseros · 2021-06-22T17:02:12Z

Hi @jroesch , @areusch ,
After further thinking we were wondering if we could merge both this PR and #8096 before merging #7518 . This is because #8096 would solve quite a few issues that we have with AoT at the moment.

What do you think?

Change-Id: I4f11da7fce1327ad89bb25f25209b57077b2c6a3

giuseros · 2021-06-24T14:58:47Z

A friendly ping @manupa-arm , @jroesch , @areusch !

areusch · 2021-06-25T23:25:32Z

@jroesch can you comment on @giuseros question above?

areusch

thanks @giuseros , i think we can merge this now. i'll review #8096 separately, but i think we are okay merging those before #7518 . depending on the complexity of the refactor, we might ask for your help migrating the logic to live in the post-TEcompiler architecture.

areusch · 2021-06-25T23:35:05Z

@manupa-arm @Mousius please take a look and explicitly approve if you're ok w/ this change

manupak

LGTM!

Mousius

And me 😸

@giuseros

This introduces structures generated to provide a documented and stable user friendly interface to a TVM generated model, as can be seen in the AOT demo application: ``` struct tvmgen_default_inputs inputs = { .input_1 = input_data, }; struct tvmgen_default_outputs outputs = { .output = output_data, }; int ret_val = tvmgen_default_run(&inputs, &outputs, NULL, NULL); ``` To facilitate this, some other changes are included: * Removed dependency on `aot_executor.{c,h}` in tests, pending the discussion in the interface RFC as to whether we keep them. * Moved creation of test DLTensor's into the AOT test utils, in future this can be replaced by loading via the Python API or otherwise * Introduce `parametrize_aot_options` which can be used to test permutations of AOT which work together - for now this filters C interface and packed operators * Updated demo application to generate the header for demonstration purposes, we should consider porting the demo application to Model Library Format and using the toolchain in the Zephyr App via CMake instead? This patch builds upon the improvements @giuseros made to AOT testing and name mangling from apache#8014

@giuseros

This introduces structures generated to provide a documented and stable user friendly interface to a TVM generated model, as can be seen in the AOT demo application: ``` struct tvmgen_default_inputs inputs = { .input_1 = input_data, }; struct tvmgen_default_outputs outputs = { .output = output_data, }; int ret_val = tvmgen_default_run(&inputs, &outputs, NULL, NULL); ``` To facilitate this, some other changes are included: * Removed dependency on `aot_executor.{c,h}` in tests, pending the discussion in the interface RFC as to whether we keep them. * Moved creation of test DLTensor's into the AOT test utils, in future this can be replaced by loading via the Python API or otherwise * Introduce `parametrize_aot_options` which can be used to test permutations of AOT which work together - for now this filters C interface and packed operators * Updated demo application to generate the header for demonstration purposes, we should consider porting the demo application to Model Library Format and using the toolchain in the Zephyr App via CMake instead? This patch builds upon the improvements @giuseros made to AOT testing and name mangling from apache#8014

@giuseros

This introduces structures generated to provide a documented and stable user friendly interface to a TVM generated model, as can be seen in the AOT demo application: ``` struct tvmgen_default_inputs inputs = { .input_1 = input_data, }; struct tvmgen_default_outputs outputs = { .output = output_data, }; int ret_val = tvmgen_default_run(&inputs, &outputs, NULL, NULL); ``` To facilitate this, some other changes are included: * Removed dependency on `aot_executor.{c,h}` in tests, pending the discussion in the interface RFC as to whether we keep them. * Moved creation of test DLTensor's into the AOT test utils, in future this can be replaced by loading via the Python API or otherwise * Introduce `parametrize_aot_options` which can be used to test permutations of AOT which work together - for now this filters C interface and packed operators * Updated demo application to generate the header for demonstration purposes, we should consider porting the demo application to Model Library Format and using the toolchain in the Zephyr App via CMake instead? This patch builds upon the improvements @giuseros made to AOT testing and name mangling from apache#8014

@giuseros

This introduces structures generated to provide a documented and stable user friendly interface to a TVM generated model, as can be seen in the AOT demo application: ``` struct tvmgen_default_inputs inputs = { .input_1 = input_data, }; struct tvmgen_default_outputs outputs = { .output = output_data, }; int ret_val = tvmgen_default_run(&inputs, &outputs, NULL, NULL); ``` To facilitate this, some other changes are included: * Removed dependency on `aot_executor.{c,h}` in tests, pending the discussion in the interface RFC as to whether we keep them. * Moved creation of test DLTensor's into the AOT test utils, in future this can be replaced by loading via the Python API or otherwise * Introduce `parametrize_aot_options` which can be used to test permutations of AOT which work together - for now this filters C interface and packed operators * Updated demo application to generate the header for demonstration purposes, we should consider porting the demo application to Model Library Format and using the toolchain in the Zephyr App via CMake instead? This patch builds upon the improvements @giuseros made to AOT testing and name mangling from apache#8014

@giuseros

This introduces structures generated to provide a documented and stable user friendly interface to a TVM generated model, as can be seen in the AOT demo application: ``` struct tvmgen_default_inputs inputs = { .input_1 = input_data, }; struct tvmgen_default_outputs outputs = { .output = output_data, }; int ret_val = tvmgen_default_run(&inputs, &outputs, NULL, NULL); ``` To facilitate this, some other changes are included: * Removed dependency on `aot_executor.{c,h}` in tests, pending the discussion in the interface RFC as to whether we keep them. * Moved creation of test DLTensor's into the AOT test utils, in future this can be replaced by loading via the Python API or otherwise * Introduce `parametrize_aot_options` which can be used to test permutations of AOT which work together - for now this filters C interface and packed operators * Updated demo application to generate the header for demonstration purposes, we should consider porting the demo application to Model Library Format and using the toolchain in the Zephyr App via CMake instead? This patch builds upon the improvements @giuseros made to AOT testing and name mangling from apache#8014

@giuseros

This introduces structures generated to provide a documented and stable user friendly interface to a TVM generated model, as can be seen in the AOT demo application: ``` struct tvmgen_default_inputs inputs = { .input_1 = input_data, }; struct tvmgen_default_outputs outputs = { .output = output_data, }; int ret_val = tvmgen_default_run(&inputs, &outputs, NULL, NULL); ``` To facilitate this, some other changes are included: * Removed dependency on `aot_executor.{c,h}` in tests, pending the discussion in the interface RFC as to whether we keep them. * Moved creation of test DLTensor's into the AOT test utils, in future this can be replaced by loading via the Python API or otherwise * Introduce `parametrize_aot_options` which can be used to test permutations of AOT which work together - for now this filters C interface and packed operators * Updated demo application to generate the header for demonstration purposes, we should consider porting the demo application to Model Library Format and using the toolchain in the Zephyr App via CMake instead? This patch builds upon the improvements @giuseros made to AOT testing and name mangling from apache#8014

@giuseros

* Introduce --interface-api={c,packed} parameter This introduces structures generated to provide a documented and stable user friendly interface to a TVM generated model, as can be seen in the AOT demo application: ``` struct tvmgen_default_inputs inputs = { .input_1 = input_data, }; struct tvmgen_default_outputs outputs = { .output = output_data, }; int ret_val = tvmgen_default_run(&inputs, &outputs, NULL, NULL); ``` To facilitate this, some other changes are included: * Removed dependency on `aot_executor.{c,h}` in tests, pending the discussion in the interface RFC as to whether we keep them. * Moved creation of test DLTensor's into the AOT test utils, in future this can be replaced by loading via the Python API or otherwise * Introduce `parametrize_aot_options` which can be used to test permutations of AOT which work together - for now this filters C interface and packed operators * Updated demo application to generate the header for demonstration purposes, we should consider porting the demo application to Model Library Format and using the toolchain in the Zephyr App via CMake instead? This patch builds upon the improvements @giuseros made to AOT testing and name mangling from #8014 * Tweak metadata variable description and MLF target loop * Remove direct usage of `relay::Var` in meta_data.h This looks like the only place that could be causing the Windows CI failures, so trying removing the additional header in meta_data.h * Linting fix * Post-rebase files fixing These tests were somehow transmuted in transit, I've updated them to the most recent variant of the test helpers. * Strip back interface API to just inputs and outputs This removes any speculative structures from the generated code and cleans up some of the documentation. * Add header guards and tweak documentation

* [AOT] Name mangling in AOT Mini-RFC is here: https://discuss.tvm.apache.org/t/mini-rfc-name-mangling-in-aot With this change we'll mangle the name of global symbols so that we can bundle together multiple models in the same application. The relay.build interface has been left unchanged, which means I am resuing mod_name as a prefix for all functions. If mod_name is None then a "_tvm" prefix is used. I had to add two different compilation functions: - _CompileEngineLowerWithModuleName to mangle all the operators with the mod_name - PartitionGraphWithModName to mangle all the operators produced by BYOC I could have changed signature of both, but that would have meant a very invasive refactoring. I refactored the aot test utils and added some tests for multiple models. Change-Id: I30e93fa075f660054577ea36cf9268ec0c6eebcb * retrigger CI Change-Id: I4f11da7fce1327ad89bb25f25209b57077b2c6a3

@giuseros

* Introduce --interface-api={c,packed} parameter This introduces structures generated to provide a documented and stable user friendly interface to a TVM generated model, as can be seen in the AOT demo application: ``` struct tvmgen_default_inputs inputs = { .input_1 = input_data, }; struct tvmgen_default_outputs outputs = { .output = output_data, }; int ret_val = tvmgen_default_run(&inputs, &outputs, NULL, NULL); ``` To facilitate this, some other changes are included: * Removed dependency on `aot_executor.{c,h}` in tests, pending the discussion in the interface RFC as to whether we keep them. * Moved creation of test DLTensor's into the AOT test utils, in future this can be replaced by loading via the Python API or otherwise * Introduce `parametrize_aot_options` which can be used to test permutations of AOT which work together - for now this filters C interface and packed operators * Updated demo application to generate the header for demonstration purposes, we should consider porting the demo application to Model Library Format and using the toolchain in the Zephyr App via CMake instead? This patch builds upon the improvements @giuseros made to AOT testing and name mangling from apache#8014 * Tweak metadata variable description and MLF target loop * Remove direct usage of `relay::Var` in meta_data.h This looks like the only place that could be causing the Windows CI failures, so trying removing the additional header in meta_data.h * Linting fix * Post-rebase files fixing These tests were somehow transmuted in transit, I've updated them to the most recent variant of the test helpers. * Strip back interface API to just inputs and outputs This removes any speculative structures from the generated code and cleans up some of the documentation. * Add header guards and tweak documentation

@giuseros

* Introduce --interface-api={c,packed} parameter This introduces structures generated to provide a documented and stable user friendly interface to a TVM generated model, as can be seen in the AOT demo application: ``` struct tvmgen_default_inputs inputs = { .input_1 = input_data, }; struct tvmgen_default_outputs outputs = { .output = output_data, }; int ret_val = tvmgen_default_run(&inputs, &outputs, NULL, NULL); ``` To facilitate this, some other changes are included: * Removed dependency on `aot_executor.{c,h}` in tests, pending the discussion in the interface RFC as to whether we keep them. * Moved creation of test DLTensor's into the AOT test utils, in future this can be replaced by loading via the Python API or otherwise * Introduce `parametrize_aot_options` which can be used to test permutations of AOT which work together - for now this filters C interface and packed operators * Updated demo application to generate the header for demonstration purposes, we should consider porting the demo application to Model Library Format and using the toolchain in the Zephyr App via CMake instead? This patch builds upon the improvements @giuseros made to AOT testing and name mangling from apache#8014 * Tweak metadata variable description and MLF target loop * Remove direct usage of `relay::Var` in meta_data.h This looks like the only place that could be causing the Windows CI failures, so trying removing the additional header in meta_data.h * Linting fix * Post-rebase files fixing These tests were somehow transmuted in transit, I've updated them to the most recent variant of the test helpers. * Strip back interface API to just inputs and outputs This removes any speculative structures from the generated code and cleans up some of the documentation. * Add header guards and tweak documentation

* [AOT] Name mangling in AOT Mini-RFC is here: https://discuss.tvm.apache.org/t/mini-rfc-name-mangling-in-aot With this change we'll mangle the name of global symbols so that we can bundle together multiple models in the same application. The relay.build interface has been left unchanged, which means I am resuing mod_name as a prefix for all functions. If mod_name is None then a "_tvm" prefix is used. I had to add two different compilation functions: - _CompileEngineLowerWithModuleName to mangle all the operators with the mod_name - PartitionGraphWithModName to mangle all the operators produced by BYOC I could have changed signature of both, but that would have meant a very invasive refactoring. I refactored the aot test utils and added some tests for multiple models. Change-Id: I30e93fa075f660054577ea36cf9268ec0c6eebcb * retrigger CI Change-Id: I4f11da7fce1327ad89bb25f25209b57077b2c6a3

Mousius requested changes May 11, 2021
View reviewed changes

giuseros mentioned this pull request May 11, 2021
Naming of the AOT executor function #7815
Closed

giuseros force-pushed the aot-name-mangling branch from 9000801 to e05eb8aCompare May 12, 2021 11:12

giuseros force-pushed the aot-name-mangling branch from 50c0572 to 71dd730Compare May 12, 2021 15:53

areusch requested changes May 13, 2021
View reviewed changes

giuseros force-pushed the aot-name-mangling branch 2 times, most recently from cbb0d21 to 18fadc8Compare May 18, 2021 21:40

manupak requested changes May 19, 2021
View reviewed changes

giuseros force-pushed the aot-name-mangling branch from d4dc20b to 501421dCompare May 24, 2021 13:07

giuseros force-pushed the aot-name-mangling branch 4 times, most recently from 4ee6f0e to 1da5b29Compare May 24, 2021 16:16

areusch requested changes May 24, 2021
View reviewed changes

giuseros force-pushed the aot-name-mangling branch 4 times, most recently from 4cd83ac to b0bcfd4Compare May 27, 2021 11:36

giuseros force-pushed the aot-name-mangling branch from b0bcfd4 to d47ed2cCompare June 1, 2021 14:51

areusch mentioned this pull request Jun 2, 2021
[AOT] Initial implementation of --unpacked-api #8023
Merged

giuseros force-pushed the aot-name-mangling branch from d47ed2c to eea84c8Compare June 3, 2021 10:38

areusch requested changes Jun 4, 2021
View reviewed changes

Comment threadsrc/relay/transforms/partition_graph.cc Outdated

giuseros force-pushed the aot-name-mangling branch 3 times, most recently from 15072dc to 469d10dCompare June 7, 2021 17:58

giuseros force-pushed the aot-name-mangling branch from 469d10d to 62171d3Compare June 7, 2021 17:59

Mousius mentioned this pull request Jun 18, 2021
Introduce --interface-api={c,packed} parameter #8280
Merged

areusch assigned jroesch Jun 22, 2021

giuseros force-pushed the aot-name-mangling branch from 62171d3 to 78e0511Compare June 22, 2021 14:27

retrigger CI
b9f6790
Change-Id: I4f11da7fce1327ad89bb25f25209b57077b2c6a3

areusch approved these changes Jun 25, 2021
View reviewed changes

manupak approved these changes Jun 28, 2021
View reviewed changes

Mousius approved these changes Jun 28, 2021
View reviewed changes

areusch merged commit 36fc525 into apache:mainJun 28, 2021

zxy844288792 mentioned this pull request Jul 21, 2021
[Bugfix] Preserve IRModule type definition and imports in NameMangleExtFuncs #8523
Merged

junrushao mentioned this pull request Nov 1, 2021
Apache TVM v0.8 Release Note Candidate #9416
Closed

Uh oh!

Conversation

giuseros commented May 10, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giuseros commented May 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

giuseros commented May 11, 2021

Uh oh!

giuseros commented May 12, 2021

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

manupak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

giuseros commented May 24, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

giuseros commented Jun 3, 2021

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

areusch commented Jun 22, 2021

Uh oh!

giuseros commented Jun 22, 2021

Uh oh!

giuseros commented Jun 22, 2021

Uh oh!

giuseros commented Jun 24, 2021

Uh oh!

areusch commented Jun 25, 2021

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

Uh oh!

areusch commented Jun 25, 2021

Uh oh!

manupak left a comment

Choose a reason for hiding this comment

Uh oh!

Mousius left a comment

giuseros commented May 10, 2021•
edited
Loading

giuseros commented May 24, 2021•
edited
Loading