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

[REFACTOR] Use more TypedPackedFuncs #2981

Merged
merged 15 commits into from
Apr 10, 2019
Merged

[REFACTOR] Use more TypedPackedFuncs #2981

merged 15 commits into from
Apr 10, 2019

Conversation

kazimuth
Copy link
Contributor

@kazimuth kazimuth commented Apr 7, 2019

This PR converts a good chunk of TVM's global PackedFuncs to be TypedPackedFuncs. Right now, this shouldn't actually change any behavior, since the type information is immediately discarded once the functions are registered in the Registry.

It does make things a lot shorter, using a few helper methods I added on Registry: set_body_simple, set_body_method, and set_body_node_method (names up for debate). These helpers use the properties of function pointers to infer argument and return types:

TVM_REGISTER_API("make.Realize")
.set_body([](TVMArgs args,  TVMRetValue *ret) {
    *ret = Realize::make(args[0], args[1], args[2], args[3], args[4], args[5]);
  }); 

becomes ->

TVM_REGISTER_API("make.Realize")
.set_body_simple(Realize::make);

This is shorter, runtime-equivalent, and also now has type information!

I don't believe this change has much runtime cost. It might interfere with inlining in some cases, would have to measure. Also, PackedFuncs aren't intended to be all that fast anyway.

So, that's the refactoring part. Now, the RFC part:

(moved to #2983)

@kazimuth kazimuth requested a review from phisiart as a code owner April 7, 2019 19:40
@kazimuth
Copy link
Contributor Author

kazimuth commented Apr 7, 2019

cc @nhynes @tqchen

@tqchen
Copy link
Member

tqchen commented Apr 7, 2019

Thanks for the contribution, I like the automated detection of the signatures. The main question is the naming convention to make sure we are not confusing the users. Let us open a new issue about other parts of RFC.

I will summarize some of my take here(which can be moved to the RFC). I like the idea of Node hierarchy compile time generation. This is something I have thought about and discussed with @jroesch for a while and might help #2523 (comment)

It is always tempting to automate more parts of the wrapper generation. However, our past experiences suggest that the automatic wrapper generation is never perfect. Think about how can we support keyword arguments, good pythonic style docstring and so on. It is also harder for developers to find the actual implementation of the "generated API" since some of them are generated at runtime. Eventually, we find that it is simpler to just do a manual wrapping, which gives us all the good native features, docs, and keep PackedFunc simple (by only support positional arguments without any meta-data).

@nhynes
Copy link
Member

nhynes commented Apr 7, 2019

This idea actually comes a lot :P #2328 (comment)

I know, for sure, that we could get good docs with really good codegen like that offered by Rust macros, but I also know for sure that we're not about to rewrite TVM in Rust :)

I think that the boilerplate really does bother new (advanced) users who want to use TVM as a tool. I wonder if there's a way forward here that satisfies all desiderata?

@kazimuth
Copy link
Contributor Author

kazimuth commented Apr 8, 2019

The main question is the naming convention to make sure we are not confusing the users.

Yeah I didn't put any thought into these, would welcome suggestions. Could do something like set_body_to_function_pointer / set_body_to_method_pointer, although those are a bit verbose.

@kazimuth kazimuth changed the title [REFACTOR][RFC] Use more TypedPackedFuncs [REFACTOR] Use more TypedPackedFuncs Apr 8, 2019
@kazimuth
Copy link
Contributor Author

kazimuth commented Apr 9, 2019

This is ready to merge once we bikeshed the helper names

@tqchen
Copy link
Member

tqchen commented Apr 9, 2019

Here are some thoughts about naming:

  • set_body_simple -> set_body_typed, since it is really a method for automatic type detection
    • I did a quick check and seems overloading do work
  • set_body_method, set_body_node_method-> set_body_method
    • Use is_base_of to detect whether a class is subclass of NodeRef or Node, and use a different specialization path.

We also need to update the comment a bit to reflect the template arguments

@tqchen tqchen added status: need review status: need update need update based on feedbacks labels Apr 9, 2019
@kazimuth
Copy link
Contributor Author

Done @tqchen

@tqchen tqchen merged commit 5178506 into apache:master Apr 10, 2019
@tqchen tqchen self-assigned this Apr 10, 2019
@tqchen tqchen added status: accepted and removed status: need update need update based on feedbacks status: need review labels Apr 10, 2019
@tqchen
Copy link
Member

tqchen commented Apr 10, 2019

Thanks @kazimuth @nhynes , this is now merged.

wweic pushed a commit to wweic/tvm that referenced this pull request May 13, 2019
* Add `set_body_simple` to Registry, refactor a lot of code to use it

* Add more types to Relay PackedFuncs

* Add Registry::set_body_method to easily make Node methods into
PackedFuncs

* Add set_body_method, set_body_node_method; start typing api_lang

* Add some docs, remove unused script

* Fix mysterious linter problem

* Touch up api_ir.cc

* Fix some issues with TOPI argument counts

* Revert changes to topi.cc to avoid problems with optional arguments

* A little more cleanup

* Type more of the api _ functions

* Whitespace

* Finalize names and docs for new registry helpers

* Update docs
wweic pushed a commit to neo-ai/tvm that referenced this pull request May 13, 2019
* Add `set_body_simple` to Registry, refactor a lot of code to use it

* Add more types to Relay PackedFuncs

* Add Registry::set_body_method to easily make Node methods into
PackedFuncs

* Add set_body_method, set_body_node_method; start typing api_lang

* Add some docs, remove unused script

* Fix mysterious linter problem

* Touch up api_ir.cc

* Fix some issues with TOPI argument counts

* Revert changes to topi.cc to avoid problems with optional arguments

* A little more cleanup

* Type more of the api _ functions

* Whitespace

* Finalize names and docs for new registry helpers

* Update docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants