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/TOPI][Op] Add TopK operator #3256

Merged
merged 15 commits into from
Jun 4, 2019
Merged

[Relay/TOPI][Op] Add TopK operator #3256

merged 15 commits into from
Jun 4, 2019

Conversation

icemelon
Copy link
Member

@icemelon icemelon commented May 29, 2019

This PR includes the following changes

  • Add topk op in Relay and Topi
  • Update argsort op to support output data type other than float32
  • Add frontend converter for topk

Thanks @yongwww for help on Tensorflow frontend converter.

@icemelon icemelon requested review from Huyuwei and Laurawly as code owners May 29, 2019 23:38
@icemelon icemelon changed the title [Relay][Op] Add TopK operator [Relay/TOPI][Op] Add TopK operator May 30, 2019
The input data tensor.

k : int, optional
Number of top elements to select. Return all elements if k < 1.
Copy link
Member

Choose a reason for hiding this comment

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

recommend returning a tensor with size 0 if k==0

Copy link
Member Author

Choose a reason for hiding this comment

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

I think current graph runtime doesn't support 0 size tensor. So I assert k >= 1 in tensorflow frontend converter.

@Laurawly
Copy link
Contributor

Have you compared you current sort implementation with existing one? Is there any performance regression for large workloads like (1, 8000)

@icemelon
Copy link
Member Author

icemelon commented Jun 1, 2019

@Laurawly I evaluated the CUDA performance of argsort with input size (1, 8000) on p3. Existing one take 28.57ms to finish, and the one in this PR takes 15.11ms.

@icemelon
Copy link
Member Author

icemelon commented Jun 3, 2019

@Laurawly @yongwww Could you review this PR again?

Copy link
Member

@yongwww yongwww left a comment

Choose a reason for hiding this comment

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

LGTM

@Laurawly Laurawly merged commit 072f8cc into apache:master Jun 4, 2019
@Laurawly
Copy link
Contributor

Laurawly commented Jun 4, 2019

Thanks @icemelon9 @yzhliu @yongwww , this is merged.

@icemelon icemelon deleted the topk branch June 4, 2019 23:30
wweic pushed a commit to wweic/tvm that referenced this pull request Jun 26, 2019
* init impl for topk

* Fix cpu for topk

* init cuda impl for topk

* Add cuda for topk

* fix

* Add doc

* update doc

* lint

* lint

* lint

* x

* fix warning

* [Relay] Add TopK in tf converter

* Add frontend converter

* fix
wweic pushed a commit to neo-ai/tvm that referenced this pull request Jun 27, 2019
* init impl for topk

* Fix cpu for topk

* init cuda impl for topk

* Add cuda for topk

* fix

* Add doc

* update doc

* lint

* lint

* lint

* x

* fix warning

* [Relay] Add TopK in tf converter

* Add frontend converter

* fix
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.

4 participants