Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Relay][TIR] Add utility to lower Relay func to TIR prim func #13606

Merged
merged 6 commits into from
Dec 14, 2022

Conversation

masahi
Copy link
Member

@masahi masahi commented Dec 13, 2022

This is very useful for creating a TensorIR program that is otherwise hard to so via manual TVMScript or te compute + create_prim_func.

A good example is a TensorIR program having AllocateConst, since it is not possible to create such modules with embedded constants via TVMScript parsing. AllocateConst has been the source of many issues and I've sent many fixes for them, but it has always been difficult to create test cases for such issues. But if we start from Relay, we can easily create such modules by lowering Relay func to TIR with link-params = True.

This also makes it possible to, say, author a complicated module in PyTorch and directly lower it to TensorIR.

As an example of its use, I added a regression test for the issue described in #13605.

@Hzfengsy @junrushao @csullivan

@tvm-bot
Copy link
Collaborator

tvm-bot commented Dec 13, 2022

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

Copy link
Contributor

@csullivan csullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, I love it. One small suggestion if you agree.

src/relay/backend/te_compiler_cache.cc Show resolved Hide resolved
Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome and very useful improvement! Thanks.

Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@masahi masahi merged commit c6652bc into apache:main Dec 14, 2022
@Mousius
Copy link
Member

Mousius commented Dec 14, 2022

👋 Hello @masahi / @csullivan / @echuraev / @junrushao,

Whilst I appreciate this is useful with the use-case demonstrated, I believe it might've needed a bit more time for others to formulate a review as it was up for less than 24 hours before merge?

One of my concerns is that this seems to be creating new public interfaces to the internals of the TECompiler which in the past we've attempted to remove:

This change means we have to maintain an interface which generates a single tir::PrimFunc from a relay::Function where-as it could potentially lead to multiple tir::PrimFunc with a single IRModule as a result. The next line down:

IRModule tir_mod = PrimFuncToIRModule(f.value());

Implies we wanted it in IRModule form anyway, so creating an IRModule with a Function and running LowerTE(mod) seems the best result here?

@masahi
Copy link
Member Author

masahi commented Dec 14, 2022

@Mousius The new function simply composes the existing interface to the TECompiler, LowerTECompute and CreatePrimFunc that converts lowered TE compute to TensorIR. LowerTECompute was introduced after the compile engine refactor you mentioned completed, and made only possible thanks to that refactoring work. So I don't get your concern here.

What I wanted to add is a simple interface that converts one Relay primitive function to the corresponding TIR prim func. Then I realized that backend/task_extraction.cc already has code that does exactly that, so I simply extracted into a reusable function and exposed it to Python. We require the input to be a primitive function, so we never generates multiple TIR prim funcs.

Implies we wanted it in IRModule form anyway, so creating an IRModule with a Function and running LowerTE(mod) seems the best result here?

This is just a convention of MetaSchedule, storing an IRModule with one prim func as a task. I didn't change any behavior about task extraction in this PR. What we want is a relay::Function -> tir::PrimFunc interface, IRModule -> IRModule is valid but it wouldn't convey what this new API is meant for. LowerTE is also overkill since I don't need to lower schedules and I'm not concerned about external codegen etc.

@Mousius
Copy link
Member

Mousius commented Dec 15, 2022

@Mousius The new function simply composes the existing interface to the TECompiler, LowerTECompute and CreatePrimFunc that converts lowered TE compute to TensorIR. LowerTECompute was introduced after the compile engine refactor you mentioned completed, and made only possible thanks to that refactoring work. So I don't get your concern here.

I find this equally concerning, given this is also exposing more of the internals of the TE Compiler rather than having clear boundaries within the codebase.

What I wanted to add is a simple interface that converts one Relay primitive function to the corresponding TIR prim func. Then I realized that backend/task_extraction.cc already has code that does exactly that, so I simply extracted into a reusable function and exposed it to Python. We require the input to be a primitive function, so we never generates multiple TIR prim funcs.

Exposing it python and as part of the public API means others can access it, which means that we have to support it as a way of using the compiler rather than as an aspect of meta-scheduler. This is similar to the previous methods that we were unable to deprecate in #13606.

This is just a convention of MetaSchedule, storing an IRModule with one prim func as a task. I didn't change any behavior about task extraction in this PR. What we want is a relay::Function -> tir::PrimFunc interface, IRModule -> IRModule is valid but it wouldn't convey what this new API is meant for. LowerTE is also overkill since I don't need to lower schedules and I'm not concerned about external codegen etc.

I don't think LowerTE is overkill in practice, if it doesn't find any external codegen or other things to do it won't take any action - it's a standardised interface between two modules in the codebase, you've already indicated meta schedule is breaking that boundary and that leads us back to the original issues with CompileEngine.

fzi-peccia pushed a commit to fzi-peccia/tvm that referenced this pull request Mar 27, 2023
…#13606)

* introduce LowerToPrimFunc to lower Relay func to TIR prim func

* add doc

* expose to python

* adding test

* another minor doc update

* Verify that the input is a primitive function
mikeseven pushed a commit to mikeseven/tvm that referenced this pull request Sep 27, 2023
…#13606)

* introduce LowerToPrimFunc to lower Relay func to TIR prim func

* add doc

* expose to python

* adding test

* another minor doc update

* Verify that the input is a primitive function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants