[CRT] Create C-runtime-style metadata module for llvm builds #7398

areusch · 2021-02-03T00:58:40Z

This PR is a follow-on to #7002 and continues its work to support BYOC functions with the llvm TVM target with the C runtime. Though the BYOC flow is supported with the TVM C++ runtime, this support depends heavily on dlsym and shared library support, neither of which are required to use the TVM C runtime.

This PR can roughly be thought of in these parts:

Build a corresponding runtime.CreateLLVMCrtMetadataModule PackedFunc to the runtime.CreateCSourceCrtMetadataModule. Also add crt here to distinguish between these metadata modules (which are intended to be compiled) and runtime::MetadataModule (which is intended to be directly loaded into the TVM compiler c++ runtime)
Move FuncRegistry and TVMSystemLibEntryPoint LLVM generator into a new LLVMModule instance. Change function registry to declare extern functions rather than using internal linkage.
Allow tvm.micro.build_static_runtime to accept object files from the LLVM backend. This temporarily solution allows us to build llvm-targeted code using microTVM.

manupak

Just a heads up that you might want to know.

manupak · 2021-02-04T12:06:02Z

src/target/metadata_module.cc

+      csource_modules.push_back(target_module);
+      target_module = CreateCSourceCrtMetadataModule(csource_modules, target);
+    } else if (target->kind->name == "llvm") {
+      binary_modules.push_back(target_module);


I think target_module should not be a binary module that gets imported inside non-DSOExportable metadata module.
I think csource_modules --> dso_modules and binary_modules --> non_dso_modules would be better. Moreover, non-DSOExportable metadata module should not be created if non of the modules need constant loading.

TL;DR
This one is little complex to explain. Let me use some definitions first (and feel free to change the names as you see fit).

[Defn.1] binary module : a runtime.Module that is not DSOExportable (not of type "c" or "llvm").
[Defn.2][NCL (needs constant loading) module : a module that requires constant loading in the init process.

As of today these two things are not strictly orthogonal. (i.e. DSOExportabe module could use constant loading -- though the only one is the example byoc c codegen).

So technically any module that implements get_func_names could be imported under the DSOExportable metadata module (llvm -- the one this PR introduces or c -- the one I introduced before). However, currently the only ones that get imported inside DSOExportable metadata module are modules that are !("binary" or "NCL"). So I had a hard time finding the name and end-up calling them binary modules.

As of today these two things are not strictly orthogonal. (i.e. DSOExportabe module could use constant loading -- though the only one is the example byoc c codegen).

Do you mean "(I.e. binary module could use constant loading..." here?

okay I think my point of confusion here (and I think it's related to lack of experience with BYOC) is: what makes metadata module non-DSO-exportable?

runtime::MetadataModuleCreate() --> is non-DSO-exportable it gets packed into to devc.{o/c} and I think we should only need this if we have modules that require constant loading. This one is of type "metadata". Im talking about the one that is created in line 96.

I mean DSOExportable modules could still use constant loading -- though I think its not very useful since we have linked params now. Only remaining place is the example c byoc codegen that uses the constant loading process as a demo to all BYOC codegens -- I think.

Thus, if none of the modules does not require constant loading -- that is indicated by arrays.empty() -- we dont need to create the non-DSOExportable metadata module.

See the diagram here : https://discuss.tvm.apache.org/t/csourcemetadata-module-a-csourcemodule-to-hold-function-registry-for-utvm/8554.

Does that make sense ?

okay so let me summarize my understanding:

at the end of the day, we want to produce a single module that imports all the others

This function gets the Module that implements target_host (target_module) plus a list of all the BYOC modules (ext_modules).

when building for the C++ runtime, we have runtime::MetadataModule for this root module. We assume each Module inext_modules contains exactly one Relay function whose name is get_symbol() and which may use ConstantNodes whose values must be initialized from get_const_vars(). get_func_names() is unused because dlsym is used to resolve functions in the C++ runtime.

when building for the C runtime, we use either the CSourceMetadataModule from Created CSourceMetaData module for model metadata #7002 or LLVMCrtMetadataModule from this PR. These are both DSO-exportable. The CSourceMetadataModule queries function names from and imports all DSO-exportable modules which don't define any const_vars. Let's call these "CRT-exportable modules" since we don't have a good name for them.

Then, any modules which are: a) non-DSO-exportable, b) not codegen'd by c or llvm backend, OR c) not defining any const_vars are imported into the resulting metadata module.

Presumably then the LLVM module should work the same way as the C-source one, the only change needed being that target->kind->name == "llvm" should also admit DSO-exportable functions to csource_modules.

Then, I'd propose the following name changes:

csource_modules -> crt_exportable_modules

binary_modules -> non_crt_exportable_modules

Yes, this seems right :)
Let me just tighten the point 5

"5.) Then, any modules which are: a) non-DSO-exportable OR b) (not defining any const_vars OR result of get_const_vars() is empty() ) are imported into the resulting (here resulting means at least one of the two conditions is met) metadata module."

Summary is 5b is covered 5a and metadata module of type "metadata" should only be created if one of the above condition is met.
Moreover, c runtime does not support ProcessModuleBlob(Im talking about : src/runtime/library_module.cc) anyway -- does it ? -- that could be used as a blanket assertion to check whether all modules are !5a and !5b if the target says c runtime ?

yeah that seems right. I think we've come to similar conclusions. I think there are a couple of larger issues to be resolved here:

When targeting the C runtime, an entirely different strategy than MetadataModule is needed to export Modules which don't run on the target_host CPU. This isn't well-defined yet. Model Library Format RFC also touches on this, but doesn't resolve it.

Related to resolving this conundrum: the model of the C++ runtime is that some library running on target_host parses binary configuration data in MetadataModule and then configures any runtime libraries or accelerators. This is generally used to implement BYOC. C runtime does not want to require any startup procedure that involves parsing binary blobs using RAM--instead, this must happen at code-generation time. This means that we need to do a better job here of organizing Module according to the DLContext which will be executing those modules.

Here we've mostly fixed problem 1. In future PR and RFC, we can work to address problem 2. Let me know if that makes sense to you!

@areusch , few clarfications :

The MetadataModule of type "metadata" is a another module that get loaded using ProcessModuleBlob in c++ runtime. Therefore, I would not agree to "MetadataModule is needed to export Modules ". For e.g., currently ARM Ethos-N that uses ProcessModuleBlob does not require MetadataModule of type "metadata" as it does not need any constants to be loaded into it in the init process. Having said that, what I agree with you is that ProcessModuleBlob should not be the way to do things in C runtime -- more specifically in uTVM environments. So you can think of MetadataModule of type "metadata" as a peer to all runtime modules that gets "packed" to use ProcessModuleBlob in the runtime.

Similarly, I would not agree "target_host parses binary configuration data in MetadataModule and then configures any runtime libraries or accelerators", because MetadataModule of type "metadata" only have the constants and other binaries go inside devc.c/o independently of MetadataModule of type "metadata".

I agree that " C runtime does not want to require any startup procedure that involves parsing binary blobs using RAM--instead, this must happen at code-generation time.".

"This means that we need to do a better job here of organizing Module according to the DLContext which will be executing those modules." -- This I agree. First thing is we need to move away from the assumption that BYOC flows always generate non-DSO-exportable runtime.Modules that is not currently true for our ongoing ARM Ethos-U work. We will be producing a DSOExportable runtime.Module. One side note to this, we might want to integrate the BYOC (possiblly not call it BYOC :) ) in the compilation pathway as it offers IRModule --> runtime.Module conversion generally and TVM's internal module compilation is "a" way of producing it. I have made a similiar comment here : #7428

So towards that goal, I think we need a mechanism to identify the DLContext without relying on their type being "c" or "llvm" as they could possiblly be coming from a BYOC flow. There should be a primary key in the runtime.Module to say its DLContext, IMHO.

Therefore, I would not agree to "MetadataModule is needed to export Modules ". For e.g., currently ARM Ethos-N that uses ProcessModuleBlob does not require MetadataModule of type "metadata" as it does not need any constants to be loaded into it in the init process.

sure, but it depends on ProcessModuleBlob calling runtime.module.loadbinary_ethos-n, right? This is the part I think of most concern going forward--for µTVM, and I think we could say more broadly for CRT (no reason to support two flows in one runtime) we want to make it possible to do as much of this logic at compile time to produce a firmware binary that requires minimal startup procedures.

So you can think of MetadataModule of type "metadata" as a peer to all runtime modules that gets "packed" to use ProcessModuleBlob in the runtime.

this makes sense to me, but I'd further qualify it to "all non-DSO-exportable runtime modules," at least for now.

Similarly, I would not agree "target_host parses binary configuration data in MetadataModule and then configures any runtime libraries or accelerators", because MetadataModule of type "metadata" only have the constants and other binaries go inside devc.c/o independently of MetadataModule of type "metadata".

You're right--I was getting this confused with codegen_blob since the FuncRegistry logic doesn't have an explicit home in C++ land.

So towards that goal, I think we need a mechanism to identify the DLContext without relying on their type being "c" or "llvm" as they could possiblly be coming from a BYOC flow. There should be a primary key in the runtime.Module to say its DLContext, IMHO.

I agree with a few clarifications. Perhaps better to discuss on the forum. The main points are: all BYOC are ext_dev, and perhaps some SoC may have a set of identical accelerators, some configured with one program and some with another. We probably should just consider how to support that case now rather than have to refactor that later--I think the difference is minor and amounts to coming up with another name for runtime::Module which indicates a) which device_type or BYOC accelerator the module is to run on and b) if multiple such things exist, a sort of "program name" for the particular accelerator program being generated.

areusch · 2021-02-12T22:41:57Z

@manupa-arm @leandron @tqchen @jroesch @gromero @tom-gall @MatthewARM @mshawcroft @u99127 @comaniac @zhiics

manupak

Thanks for hard work!
Broadly LGTM, just a one clarification about a test.

manupak · 2021-02-16T06:25:49Z

tests/python/unittest/test_crt.py

+    global TARGET
+    old_target = TARGET
+    try:
+        TARGET = tvm.target.Target("llvm " + str(TARGET)[2:])


Not very clear to me what is going on here ?
I would imagine we would want to test both llvm and c targets, right ?
May I ask why is the need for try-finally here?

clarified a bit--i'm being lazy and reusing test_compile_runtime

zhiics

The changes for metadata module and csource module look good to me, please fix the ci

python/tvm/micro/compiler.py

manupak

LGTM

areusch · 2021-02-22T18:44:31Z

@zhiics merge?

tmoreau89 · 2021-02-23T17:55:00Z

Thank you @areusch @zhiics @manupa-arm the PR has been merged.

…7398) * Create C-runtime-style metadata module for llvm builds. * maybe address manupa's comment * lint * actually address manupa comments * comment and rename * git-clang-format * pylint * cpp warning * try to fix apps/bundle_deploy * black format * build correct file * Use save() for C++-runtime targeted artifacts. * fix build_module LLVM metadata module conditions * fix test comment * black format * further restrict CRT MetadataModule creation * Fix test_link_params * black format and address zhiics comments * fix test_link_params, i think?

trevor-m · 2021-03-02T22:53:54Z

FYI, this appears to break TVM builds with USE_LLVM=OFF. Will try to come up with a fix

Create C-runtime-style metadata module for llvm builds.

18a54ba

areusch force-pushed the llvm-metadata-module branch from dad84a0 to 18a54ba Compare February 3, 2021 01:00

manupak reviewed Feb 4, 2021

View reviewed changes

areusch added 11 commits February 5, 2021 14:07

maybe address manupa's comment

f4d4d77

lint

48b005d

actually address manupa comments

eae7a69

comment and rename

87c7ca5

git-clang-format

aa89625

pylint

fd651da

cpp warning

567407a

try to fix apps/bundle_deploy

5f8b4a2

black format

2b53a10

build correct file

67fc137

Use save() for C++-runtime targeted artifacts.

d026b07

areusch marked this pull request as ready for review February 12, 2021 22:41

manupak reviewed Feb 16, 2021

View reviewed changes

areusch added 6 commits February 16, 2021 09:50

Merge remote-tracking branch 'origin/main' into llvm-metadata-module

cca239a

fix build_module LLVM metadata module conditions

461cb26

fix test comment

470851a

black format

7a3be65

further restrict CRT MetadataModule creation

7ef66ef

Fix test_link_params

aed8ebc

zhiics reviewed Feb 18, 2021

View reviewed changes

python/tvm/micro/compiler.py Outdated Show resolved Hide resolved

areusch added 2 commits February 18, 2021 08:15

black format and address zhiics comments

6c508f2

fix test_link_params, i think?

ce38ed1

zhiics approved these changes Feb 19, 2021

View reviewed changes

manupak approved these changes Feb 19, 2021

View reviewed changes

tmoreau89 merged commit 0ba3741 into apache:main Feb 23, 2021

This was referenced Mar 9, 2021

Fix TVM compilation with USE_LLVM=OFF neo-ai/tvm#194

Closed

Fix TVM compile without LLVM #7621

Merged

junrushao mentioned this pull request Nov 1, 2021

Apache TVM v0.8 Release Note Candidate #9416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CRT] Create C-runtime-style metadata module for llvm builds #7398

[CRT] Create C-runtime-style metadata module for llvm builds #7398

areusch commented Feb 3, 2021

manupak left a comment

manupak Feb 4, 2021 •

edited

Loading

areusch Feb 4, 2021 •

edited

Loading

manupak Feb 4, 2021 •

edited

Loading

areusch Feb 6, 2021

manupak Feb 6, 2021

areusch Feb 12, 2021

manupak Feb 15, 2021 •

edited

Loading

areusch Feb 16, 2021 •

edited

Loading

areusch commented Feb 12, 2021 •

edited

Loading

manupak left a comment

manupak Feb 16, 2021

areusch Feb 16, 2021

zhiics left a comment

manupak left a comment

areusch commented Feb 22, 2021

tmoreau89 commented Feb 23, 2021

trevor-m commented Mar 2, 2021

[CRT] Create C-runtime-style metadata module for llvm builds #7398

[CRT] Create C-runtime-style metadata module for llvm builds #7398

Conversation

areusch commented Feb 3, 2021

manupak left a comment

Choose a reason for hiding this comment

manupak Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

areusch Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

manupak Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

areusch Feb 6, 2021

Choose a reason for hiding this comment

manupak Feb 6, 2021

Choose a reason for hiding this comment

areusch Feb 12, 2021

Choose a reason for hiding this comment

manupak Feb 15, 2021 • edited Loading

Choose a reason for hiding this comment

areusch Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

areusch commented Feb 12, 2021 • edited Loading

manupak left a comment

Choose a reason for hiding this comment

manupak Feb 16, 2021

Choose a reason for hiding this comment

areusch Feb 16, 2021

Choose a reason for hiding this comment

zhiics left a comment

Choose a reason for hiding this comment

manupak left a comment

Choose a reason for hiding this comment

areusch commented Feb 22, 2021

tmoreau89 commented Feb 23, 2021

trevor-m commented Mar 2, 2021

manupak Feb 4, 2021 •

edited

Loading

areusch Feb 4, 2021 •

edited

Loading

manupak Feb 4, 2021 •

edited

Loading

manupak Feb 15, 2021 •

edited

Loading

areusch Feb 16, 2021 •

edited

Loading

areusch commented Feb 12, 2021 •

edited

Loading