-
Notifications
You must be signed in to change notification settings - Fork 96
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
Address issues of top-k op #16670
Draft
asandhupatlaTT
wants to merge
16
commits into
main
Choose a base branch
from
asandhupatla/13235
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Address issues of top-k op #16670
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
c8c2e3b
add skeleton support for largest & sorted
asandhupatlaTT 3d67320
potential patch for k = 64
asandhupatlaTT bf1a99b
k=32 passes but for k=64 faiks: patch1
asandhupatlaTT 212eb7b
missing one last sort in rebuild
asandhupatlaTT 5a65a8f
passes for all cases except when W=8192
asandhupatlaTT 953d389
add test cases for testing
asandhupatlaTT 7a0ee2d
fix bug when we rebuild same tile twice at same time
asandhupatlaTT d146cc0
single core top-k works for k=64
asandhupatlaTT e9288d1
patch 1 for multicore support
asandhupatlaTT 6743b03
fix bug where we get wrong writer pointer for k > 32
asandhupatlaTT cab6201
support all dims
asandhupatlaTT 67772b1
fix test cases of top-k
asandhupatlaTT 71d4360
cleanup
asandhupatlaTT 4f86077
add sorted flags to test case
asandhupatlaTT 1f3d380
support K which is not power of 2
asandhupatlaTT 9d4eef9
support n-d tensor
asandhupatlaTT File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The issue mentions a k of 50. That should be tested as well.
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.
Yes i tried k=50 but If i'm not mistaken, tt-budha (& thereby my code) only supports powers of 2.
Ill see what needs to be done to support non-powers-of-2 numbers.
One idea is : convert K to nearest power of 2 --> do LLK/compute kernel --> then reshape or slice output to desired shape
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.
It would be easiest to use the separation of ExecuteTopK and TopK and do what you suggested, except convert K to either 32 or 64.
ExecuteTopK:
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.
It is correct that the TopK algorithm only supports powers of 2 for K. Any non-power-2 values need to be rounded UP to the nearest supported K value, and then you can truncate the output if needed. Rounding down won't work.
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.
@asandhupatlaTT I see only 32 and 64 in tests/ttnn/unit_tests/operations/test_topk.py
What happens when you try 2, 4, 8, and 16?
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.
@bbradelTT since my kernel is. similar to tt-budha, it should work. but ill few test cases in next patch