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] Remove kCompiler attr from ext mod #5028

Closed
wants to merge 2 commits into from

Conversation

mbaret
Copy link
Contributor

@mbaret mbaret commented Mar 10, 2020

Functions in external modules kept their kCompiler attribute which means UseDefaultCompiler returns false. This prevents you from being able to run any passes on these external modules. By setting this attribute to 'default' when we add the function to the external module, we allow for passes to be run external modules.

Functions in external modules kept their kCompiler
attribute which means UseDefaultCompiler returns false.
This prevents you from being able to run any passes on
these external modules. By setting this attribute to
'default' when we add the function to the external
module, we allow for passes to be run external modules.

Change-Id: Ia60de3f0b0db9101738eac407aca4913fad9525a
@mbaret
Copy link
Contributor Author

mbaret commented Mar 10, 2020

cc @zhiics @tqchen

src/relay/backend/compile_engine.cc Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented Mar 20, 2020

cc @zhiics

@tqchen tqchen added status: need review status: need update need update based on feedbacks labels Mar 20, 2020
@zhiics
Copy link
Member

zhiics commented Mar 20, 2020

Sorry. I totally missed this PR.

@@ -627,6 +627,7 @@ class CompileEngineImpl : public CompileEngineNode {
const tvm::tir::StringImmNode* symbol_name = ext_symbol.as<tvm::tir::StringImmNode>();
CHECK(symbol_name) << "No external symbol is set for:\n" << AsText(src_func, false);
auto gv = GlobalVar(symbol_name->value);
src_func = WithAttr(src_func, attr::kCompiler, tir::StringImmNode::make("default"));
Copy link
Member

Choose a reason for hiding this comment

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

Now you should not give it "default" but a null string ObjectRef, so that the returned value is not defined

https://github.com/apache/incubator-tvm/blob/0a0e58bfa4c87b2cbff0be2b401da0b3a08fcfe8/include/tvm/ir/function.h#L99

Copy link
Member

Choose a reason for hiding this comment

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

Also, inorder to use CoW, you may want to use src_func = WithAttr(std::move(src_func), ...)

@tqchen
Copy link
Member

tqchen commented Apr 3, 2020

please followup, or close the PR if it is superseded by the later ones

@zhiics zhiics closed this Apr 3, 2020
@zhiics
Copy link
Member

zhiics commented Apr 3, 2020

Let's close it. I think it is not needed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: need review status: need update need update based on feedbacks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants