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.
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
[SYCLomatic] Block Store headers core #1819
[SYCLomatic] Block Store headers core #1819
Changes from 8 commits
13d8b67
7517519
6b7fd09
454c453
9e75c62
ffbd181
a0007e1
49147b8
18f826a
7149372
431d4a4
8cc73f1
a677eb2
98d0193
79295f8
c4fe035
76ec684
41b1c8a
b046dcc
f86801d
273d098
3185ceb
cc00403
56c07e1
28ff868
e87c0a6
1802fbe
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I'd like to have a more detail comments like
SYCLomatic/clang/runtime/dpct-rt/include/dpct/sparse_utils.hpp
Lines 278 to 306 in b2e5588
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.
get_global_range
returns the global dimensions of the kernel, so when launching more than a single work-group this will be incorrect.We can switch this to what we did in group load:
size_t group_work_items = item.get_local_range().size();
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.
I'd recommend making sure we have testing coverage for such a case 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 local range should be used, all the tests are currently in 1 wg . Will extend tests for other wg sizes in a separate PR. thanks.
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.
I'm not sure what this comment means exactly. But also, lets use our own terminology here.
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.
Can you describe what you mean by "range loading" means in this context?
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.
Range loading/storing refers to loading/storing within bounded intervals across the warps. Not in the full scope .