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

Initial Implementation of TIRToRuntime Target hook #9190

Merged
merged 3 commits into from
Oct 11, 2021

Conversation

Mousius
Copy link
Member

@Mousius Mousius commented Oct 4, 2021

This is the initial implementation which wires in a test case for TIRToRuntime, in order to get this working I re-used CodegenCHost as it implements all of the Ops required from the lowered PrimFunc.

Currently, the IRModule is non-unified but in future work it should definitely do so, I wanted to implement the basics here to get the infra in place.

The example now shows how to register the target, and customize the pipeline with minimal additional work involved 😸

This is the initial implementation which wires in a test case for TIRToRuntime, in order to get this working I re-used `CodegenCHost` as it implements all of the `Op`s required from the lowered `PrimFunc`.

Currently, the `IRModule` is non-unified but in future work it should definitely do so, I wanted to implement the basics here to get the infra in place.
@areusch
Copy link
Contributor

areusch commented Oct 5, 2021

@@ -34,6 +34,8 @@
#include <mutex>
#include <stack>

#include "../relay/backend/te_compiler.h"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it is needed in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good spot, this was when I was trying to bundle building a unified IRModule into this PR - I'll fix 😸

@Mousius Mousius requested a review from Hzfengsy October 6, 2021 09:58
@Hzfengsy
Copy link
Member

Hzfengsy commented Oct 6, 2021

Thanks @Mousius. It looks good to me.
However, I'm not the expert of this part. So, sorry I can not approve it. Let's wait for reviews from other committers.

Copy link
Contributor

@mbs-octoml mbs-octoml left a comment

Choose a reason for hiding this comment

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

LGTM Just a nit -- if CI is green prob not even worth worrying about. Thanks!

// can be used alongside the default host codegen based on device type
// this is so the correct code generator is used later instead of overriding the target.
// We need better support for inserting multiple kDLCPU targets as our current options
// are kDeviceKernelLaunch or not
Copy link
Contributor

Choose a reason for hiding this comment

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

sigh. we'll get there.

@@ -401,12 +401,21 @@ std::pair<IRModule, IRModule> SplitDevHostFuncs(IRModule mod_mixed, const Target
auto opt_mixed = transform::Sequential(mixed_pass_list);
mod_mixed = opt_mixed(std::move(mod_mixed));

// We make an assumption here that the overriden host target
Copy link
Contributor

Choose a reason for hiding this comment

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

Heads up that @mikepapadim is working here in #9103

} // namespace contrib
} // namespace relay

TVM_REGISTER_TARGET_KIND("example_target_hook", kDLCPU)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: example_target_kind
And maybe a comment -- 'here we register blah blah'.

@Mousius
Copy link
Member Author

Mousius commented Oct 11, 2021

@mbaret could you take a look, @mbs-octoml seems happy so I'm happy 😸 will look to follow up in a later PR on the minor nit.

Copy link
Contributor

@mbaret mbaret left a comment

Choose a reason for hiding this comment

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

LGTM

@mbaret mbaret merged commit 0b91e53 into apache:main Oct 11, 2021
@mbaret
Copy link
Contributor

mbaret commented Oct 11, 2021

Thanks @Mousius, @mbs-octoml this is merged

@Mousius Mousius deleted the tir-to-runtime branch October 11, 2021 13:47
masahi pushed a commit to Laurawly/tvm-1 that referenced this pull request Oct 14, 2021
* Initial Implementation of TIRToRuntime Target hook

This is the initial implementation which wires in a test case for TIRToRuntime, in order to get this working I re-used `CodegenCHost` as it implements all of the `Op`s required from the lowered `PrimFunc`.

Currently, the `IRModule` is non-unified but in future work it should definitely do so, I wanted to implement the basics here to get the infra in place.

* Fix heterogeneous compute with multiple kDLCPU targets

* Remove rogue te_compiler.h include
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
* Initial Implementation of TIRToRuntime Target hook

This is the initial implementation which wires in a test case for TIRToRuntime, in order to get this working I re-used `CodegenCHost` as it implements all of the `Op`s required from the lowered `PrimFunc`.

Currently, the `IRModule` is non-unified but in future work it should definitely do so, I wanted to implement the basics here to get the infra in place.

* Fix heterogeneous compute with multiple kDLCPU targets

* Remove rogue te_compiler.h include
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
* Initial Implementation of TIRToRuntime Target hook

This is the initial implementation which wires in a test case for TIRToRuntime, in order to get this working I re-used `CodegenCHost` as it implements all of the `Op`s required from the lowered `PrimFunc`.

Currently, the `IRModule` is non-unified but in future work it should definitely do so, I wanted to implement the basics here to get the infra in place.

* Fix heterogeneous compute with multiple kDLCPU targets

* Remove rogue te_compiler.h include
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.

5 participants