-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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] Refactor - Move infer types to a header file. #3783
Conversation
In these cases, perhaps it makes sense to keep the files as they are but only add declarations as internal header |
I was also thinking about it. But, we found that dialect operators can have their own new Attrs (like QnnConv2D Attrs). These infer type functions are dependent on these attrs types. This means that we have to make the infer type functions templated, while having the same default implementation for both attrs (Conv2DAttrs and QnnConv2D Attrs). In that case, it would make sense to keep the implementation in h file because of templates. Relevant line - https://github.com/dmlc/tvm/pull/3580/files#diff-077304c74fe3bf38826eb12eb52ff29cR106 (I have also made the infer function templated there) |
src/relay/op/tensor/transform.h
Outdated
namespace tvm { | ||
namespace relay { | ||
|
||
bool CastRel(const Array<Type>& types, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if moving to header we will need to inline all these methods
@tqchen let me know if you have other ideas. With templated functions, it seems we have to put the implementation and declaration at the same place. |
Perhaps I'm missing something, but if these particular functions are not templated (I couldn't see the |
Thanks @slyubomirsky for interest. The templates arise when we use Dialects and want to share the InferType functionality. This is happening in QNN dialect for example for conv2d operator. We have new Attrs - Relevant line - https://github.com/dmlc/tvm/pull/3580/files#diff-077304c74fe3bf38826eb12eb52ff29cR106 (I have also made the infer function templated there) I have also updated this PR to show how this will look like if we go this path. |
Ah, thanks for the explanation. |
Ping folks for discussion and next action items |
Ping @tqchen @yzhliu @zhiics @vinx13 @slyubomirsky Given that this has been idle for few days, I am guessing that there is an unease in making this big of a change in Relay codebase. How about an alternative then? If we dont want to share the InferType (and possibly others in future) among Dialects, Dialect ops will have to write their own InferType functions. For QNN ops, it is a co-incidence that the inferType functionality is same. Currently, there are around 8-10 QNN ops that we are targetting. We can copy paste the infer-type for these ops from Relay to QNN and wrap them in a different function. Here, there is a code duplication, but with much less impact on Relay codebase. What do you guys think? |
Another alternative could be to move only the needed ops into the header. Lesser footprint. |
I would think having some code duplication in this case is okay since the added code is a dialect which is a quite small subset of Relay ops. Otherwise, we may need to move all XXRel around to keep consistency. |
This is also blocking @anijain2305's other PRs for a while, like #3819, #3736, etc. We should probably have some consensus to unblock these PRs. |
+1 for @shoubhik 's suggestion. I feel whether to make InferType function templated does not necessarily to be an either/or question. |
Summary of the discussions: The problem is that the current implementation of InferType is in the cc file. @anijain2305 needs some of them for qnn ops. To use the same InferType function for different types, we can template them. But the default implementation should be in the header file. Or we can have specializations in different source files. So there are two major discussions here
While all folks in the discussion don't have strong preference between these two approaches, it looks that the first former is more favorable to more ppl here. I would suggest we go the first approach and move forward. |
9972be5
to
7b6bd05
Compare
Thanks everyone. This is now merged. |
Moving the infer types to a header file. This helps in sharing infer type functions across Relay and dialects (like QNN). Current example is only for transform.h to just show the changes.