[AOT] Introducing AOT in TVM

[giuseros]

This change adds the code generation and minimal runtime API to use the
Ahead Of Time (AOT) compilation flow. The main logic is contained in:

• src/relay/backend/aot_codegen.cc

Which produces a TIR PrimFunc traversing the Relay graph

The runtime interface (authored by @Mousius) leaves a gap for future
iterations using platform-specific features from RTOS.

Currently AOT runs successfully on x86 in a host OS, running these
tests on micro is coming soon.

This PR is based on the RFC described here: https://discuss.tvm.apache.org/t/implementing-aot-in-tvm/9206

Co-authored-by: Christopher Sidebottom Christopher.Sidebottom@arm.com

[giuseros]

cc: @u99127@mshawcroft@areusch@Mousius@MatthewARM@manupa-arm

[jwfromm]

Thanks for this awesome contribution @giuseros, its definitely a big milestone for the project. I looked through your changes and didn't find any docs or tutorials on how to use the AOT. Although there are some tests that show it being used, I think a doc would go a long way for encouraging others to use this approach.

[areusch]

great work @giuseros and thanks for your contribution! I left a bunch of comments, many for my understanding--so please don't take them as an over-critique of the code. I think this is definitely going in the right direction and I think most of the things to debate here are specifics.

it looks like there are a couple of renames that have landed since you've been working on this:

• DLDevice rename
• GraphExecutor rename

would be great if you could update the PR to include those changes too.

[giuseros]

Hi @areusch , just back from holidays.

First of all, thank you so much to be so thorough in the review. I appreciate this.

Instead of a big single reply, I will gradually reply to your comments

[giuseros]

Hi @jwfromm ,
I am happy to add docs for AOT. Could you just point me to an example of what you mean by doc/tutorial? Where should it go in the codebase?

Thanks!

[manupak]

Hi @giuseros ,

Thanks and looking good.
I did an initial pass of at the export_library_format and build_module.py changes. I will look at the rest in another pass.

[giuseros]

Hi all,
I refactored bits and pieces. In particular:

• Moved everything in crt (with the agreement that in the next future we will remove dependencies on things that embedded platforms might struggle with)
• Refactored the model_library_format.py and the executor factories to be a bit more generic and elegant. Please, let me know what you think

[areusch]

@giuseros did another pass. GH decided that many of my previous comments are now outdated--I know best practice is to force-push iterations on your PR, but can you use git merge to sync it rather than force-pushing? I think there's enough open comments here I won't be able to keep track of them otherwise.

cc @tqchen -- we need to address this limitation with our code review tool.

[giuseros]

Hi @tqchen , @areusch ,
I updated a new patch while we agree on the codegen changes here.

[areusch]

@giuseros sorry for the delay, lots to review and stuff. I commented on our codegen thread, let's agree on a path forward for this PR, then we can launch some separate discussions about the pieces we're deferring.

[areusch]

thanks @giuseros for your work on this, I think it's shaping up! took another look and replied to our mega-comment on PR direction.

[giuseros]

Hi @areusch ,
Thanks for you comments. I left some questions where I didn't get exactly what you meant (sorry :) ). The major things I will do next week is:

• Introduce a new intrinsic to support the cpacked_func call
• Move the aot_executor.h in an internal folder, so that is not a public interface and update the aot_test.mk make file accordingly

[manupak]

Hi @giuseros ,

Did a second pass -- my comments are mostly based on the structure and mostly around overloading with graph executor codegen. See if you agree.

[giuseros]

Hi @manupa-arm , @areusch ,
I applied most of the changes. Namely:

• Moved to a call_cpacked builtin
• Moved the aot_executor.h in internal
• Moved to use tir::Allocate
• Removed the aot_backend_api.c (and defined a fake TVMFuncRegisterGlobal in test.c)
• Removed the runner function PrimFunc returned as internal representation by AOT
  I partially addressed the following:
• About unification of BuildOutput , @manupa-arm we agreed that it is too complex to do now, because of the get_graph_json method defined in the RelayBuildModule.
• Instead of passing the LoweredOutput to the CreateMetadataModule I moved AOTMetadata to a generic Metadata and used it. The main reason is that otherwise I need to expose to the Object system the LoweredOutput class, which seems to be too much of an internal back-end class to be exposed.

Please, let me know what you think!

[manupak]

Just few nits and a suggestion to add a test case to check BYOC compilation pathway.
Its looking good @giuseros .

[areusch]

here's another review pass, I think this is looking good. couldn't reply to your executor param comment thread while doing this review, will try after I hit submit review.

[giuseros]

Hi @areusch ,
I worked on your suggestions, this is what I did:

• Moved meta_data.h in include/tvm/runtime. This is to use the runtime::kTvmExecutor{Graph,Aot} also from tests, and in this way we don't need to include relative paths in the files.
• Added the stack FIFO memory checks. We are "wasting" 4 bytes per block, so I enabled it only if TVM_CRT_RUNTIME is defined.
• In the Executor factory, I merged get_internal_repr and save_executor_config in a single get_executor_config
• Added tests for the content of sid sizes.
  Only thing I wasn't able to do was to add tests for FIFO enforcement. I have a test working in my local repo, but the problem is that I am not able to define TVM_CRT_RUNTIME for the single aot_memory_test.cc (the best would be to define it for the single test). Any suggestions?

[areusch]

@giuseros great! took another look here. here's my thoughts:

• maybe we could just move the constants in meta_data.h into the include tree? the bar for what should be there is "is it user-facing?" I think those two constants are definitely user-facing but nothing in this change promotes the other structs/classes declared there.
• I left some comments about when to enable but the impl looks fine.
• merging the two seems fine. left some comments on the top-level ExecutorFactory interface.
• I sort of think the final problem could be fixed by just turning the check on all the time. alternatively, you could create a crt_config.h for testing and update the test include path

[giuseros]

Hi @areusch ,
I addressed most of the comments except the stack_allocator. Let's discuss a bit more on the thread here

[giuseros]

Hi @areusch ,
I reworked most of the comments except the ones concerning the stack allocator where the discussion is still going on

[areusch]

@manupa-arm please take another look and explicitly approve if you're okay with this!
https://tvm.apache.org/docs/contribute/code_review.html#approve-and-request-changes-explicitly

[manupak]

LGTM

[areusch]

thanks @giuseros for your hard work on this effort and for bearing with me through a long review! and thanks @manupa-arm@jroesch@tqchen@tkonolige@jwfromm for helping to review the PR!