Skip to content

[NODE][REFACTOR] Rename IRFunctor->NodeFunctor, use func pointer - #4247

Merged
tqchen merged 3 commits into
apache:masterfrom
tqchen:node
Nov 1, 2019
Merged

[NODE][REFACTOR] Rename IRFunctor->NodeFunctor, use func pointer#4247
tqchen merged 3 commits into
apache:masterfrom
tqchen:node

Conversation

@tqchen

@tqchen tqchen commented Nov 1, 2019

Copy link
Copy Markdown
Member

Previously we used std::function for the functor dispatching.
It introduces additional overhead and problems during dll destruction(of std::function).

This PR changes the std::function to function pointers.
This change a bit restrictions around the set_dispatch that we can get around,
but will improve the general efficiency by reducing one level of indirection in the std::function.
We also no longer need special marcos to register functions to the Functor.

…for dispatching.

Previously we used std::function for the functor dispatching.
It introduces additional overhead and problems during dll destruction(of std::function).

This PR changes the std::function to function pointers.
This change a bit restrictions around the set_dispatch that we can get around,
but will improve the general efficiency by reducing one level of indirection in the std::function.
We also no longer need special marcos to register functions to the Functor.
@tqchen

tqchen commented Nov 1, 2019

Copy link
Copy Markdown
Member Author

cc @zhiics @jroesch @icemelon9

@zhiics zhiics left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, only some nitpicks.

Comment thread include/tvm/node/functor.h Outdated
*/
R operator()(const ObjectRef& n, Args... args) const {
uint32_t type_index = n->type_index();
CHECK(type_index < func_.size() &&

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CHECK(can_dispath(n))?

Comment thread include/tvm/node/functor.h Outdated
return type_index < func_.size() && func_[type_index] != nullptr;
}
/*!
* \brief invoke the functor , dispatch on type of n

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
* \brief invoke the functor , dispatch on type of n
* \brief invoke the functor, dispatch on type of n

Comment thread include/tvm/node/functor.h Outdated
*
* \code
* // Use NodeFunctor to implement IRPrinter similar to Visitor Pattern.
* // vtable allows easy patch in of new Node types, without changing

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
* // vtable allows easy patch in of new Node types, without changing
* // vtable allows easy patch of new Node types, without changing

@tqchen tqchen changed the title [NODE][REFACTOR] Rename IRFunctor->NodeFunctor, use function pointe [NODE][REFACTOR] Rename IRFunctor->NodeFunctor, use func pointer Nov 1, 2019
@tqchen

tqchen commented Nov 1, 2019

Copy link
Copy Markdown
Member Author

Thanks @zhiics i have updated per your comment

Comment thread include/tvm/node/functor.h Outdated
CHECK(can_dispatch(n))
<< "NodeFunctor calls un-registered function on type "
<< n->GetTypeKey();
return (*func_[type_index])(n, std::forward<Args>(args)...);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

s/type_index/n->type_index()

This is probably the error.

@zhiics zhiics left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@tqchen
tqchen merged commit 9a3d2ec into apache:master Nov 1, 2019
@tqchen

tqchen commented Nov 1, 2019

Copy link
Copy Markdown
Member Author

Thanks @zhiics

zxy844288792 pushed a commit to neo-ai/tvm that referenced this pull request Nov 13, 2019
…che#4247)

* [NODE][REFACTOR] Rename IRFunctor->NodeFunctor, use function pointer for dispatching.

Previously we used std::function for the functor dispatching.
It introduces additional overhead and problems during dll destruction(of std::function).

This PR changes the std::function to function pointers.
This change a bit restrictions around the set_dispatch that we can get around,
but will improve the general efficiency by reducing one level of indirection in the std::function.
We also no longer need special marcos to register functions to the Functor.
@tqchen
tqchen deleted the node branch December 23, 2019 23:39
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.

2 participants