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.

Sign up for GitHub

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

Jump to bottom

[NNVM] Move FTVMCompute registration of the elementwise operator to c++ #1351

Merged
merged 3 commits into from
Jun 29, 2018
Merged

[NNVM] Move FTVMCompute registration of the elementwise operator to c++ #1351

merged 3 commits into from
Jun 29, 2018

Conversation

nishi-t
Copy link
Contributor

@nishi-t nishi-t commented Jun 28, 2018 •
edited
Loading

This PR move following operator's registration to c++

  • full
  • full_like
  • zeros
  • zeros_like
  • ones
  • ones_like

Please review.

srkreddy1238
srkreddy1238 requested changes Jun 28, 2018
View reviewed changes
Copy link
Contributor

@srkreddy1238 srkreddy1238 left a comment

Choose a reason for hiding this comment

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

Do you think we should use out->dtype instead of Float(32) for fill_value ?

@nishi-t
Copy link
Contributor Author

nishi-t commented Jun 29, 2018

@srkreddy1238 Thank you for your suggestion. Yes, it looks good better of it. Would you review again?

@srkreddy1238
Copy link
Contributor

@nishi-t
Just noticed InitOpWithScalarParam and InitOpParam has a dtype inside, hence full, zeros and ones should use it. Others can use out->dtype.

Thanks.

@nishi-t
Copy link
Contributor Author

nishi-t commented Jun 29, 2018

@srkreddy1238 Thanks! I agreed and fixed it.

srkreddy1238
srkreddy1238 reviewed Jun 29, 2018
View reviewed changes
nnvm/src/top/tensor/elemwise.cc
"FTVMCompute", [](const NodeAttrs& attrs,
const Array<Tensor>& inputs,
const Array<Tensor>& out_info) {
const ElementWiseReduceParam& param = nnvm::get<ElementWiseReduceParam>(attrs.parsed);
Copy link
Contributor

Choose a reason for hiding this comment

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

@tqchen
We dropped num_args in topi. Do we need here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that num_args cannot be dropped from elemwise_sum in nnvm. Because, num_args is used in here.

srkreddy1238
srkreddy1238 approved these changes Jun 29, 2018
View reviewed changes
Copy link
Contributor

@srkreddy1238 srkreddy1238 left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM.

@tqchen tqchen merged commit ebdde3c into apache:master Jun 29, 2018
@nishi-t nishi-t mentioned this pull request Jul 2, 2018
Merged
tqchen pushed a commit to tqchen/tvm that referenced this pull request Jul 6, 2018
@nishi-t @tqchen
[NNVM] Move FTVMCompute registration of the elementwise operator to c…
b6ead6d 
…++ (apache#1351)
mnuyens pushed a commit to mnuyens/tvm that referenced this pull request Jul 10, 2018
@nishi-t @tqchen
[NNVM] Move FTVMCompute registration of the elementwise operator to c…
ab0d186 
…++ (apache#1351)
sergei-mironov pushed a commit to sergei-mironov/tvm that referenced this pull request Aug 8, 2018
@nishi-t @sergei-mironov
[NNVM] Move FTVMCompute registration of the elementwise operator to c…
c3672db 
…++ (apache#1351)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tqchen tqchen tqchen approved these changes

@srkreddy1238 srkreddy1238 srkreddy1238 approved these changes

Assignees
No one assigned
Labels
status: accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nishi-t @srkreddy1238 @tqchen