This repository was archived by the owner on Mar 21, 2024. It is now read-only.
Migrate kernel specialization infra to ptx_dispatch#1735
Closed
alliepiper wants to merge 7 commits intoNVIDIA:mainfrom
Closed
Migrate kernel specialization infra to ptx_dispatch#1735alliepiper wants to merge 7 commits intoNVIDIA:mainfrom
ptx_dispatch#1735alliepiper wants to merge 7 commits intoNVIDIA:mainfrom
Conversation
Collaborator
Author
|
run tests |
miscco
reviewed
Jul 5, 2022
miscco
reviewed
Jul 5, 2022
Collaborator
Author
|
run tests |
Collaborator
Author
|
run tests |
3 similar comments
Collaborator
Author
|
run tests |
Collaborator
Author
|
run tests |
Collaborator
Author
|
run tests |
alliepiper
force-pushed
the
if_target
branch
3 times, most recently
from
e04685b to
a7d19dc
Compare
Collaborator
Author
|
run tests |
alliepiper
force-pushed
the
if_target
branch
from
a7d19dc to
67b370d
Compare
Collaborator
Author
|
run tests |
alliepiper
force-pushed
the
if_target
branch
from
67b370d to
fb3dae9
Compare
Collaborator
Author
|
run tests |
alliepiper
force-pushed
the
if_target
branch
from
fb3dae9 to
28ddfca
Compare
Collaborator
Author
|
run tests |
alliepiper
force-pushed
the
if_target
branch
from
28ddfca to
99cdc8e
Compare
Collaborator
Author
|
run tests |
alliepiper
added
type: enhancement
alliepiper
added
the
release: breaking change
alliepiper
force-pushed
the
if_target
branch
from
99cdc8e to
25b4b04
Compare
Collaborator
Author
|
run tests |
alliepiper
force-pushed
the
if_target
branch
2 times, most recently
from
0002fc2 to
c86a3b7
Compare
Collaborator
Author
|
Resolved conflicts, restarting CI for new CUB tests. run tests |
alliepiper
force-pushed
the
if_target
branch
2 times, most recently
from
05c3527 to
a78f492
Compare
Collaborator
Author
|
run tests |
alliepiper
force-pushed
the
if_target
branch
from
a78f492 to
cab82b7
Compare
Collaborator
Author
|
run tests |
alliepiper
force-pushed
the
if_target
branch
from
cab82b7 to
b869c2c
Compare
Collaborator
Author
|
run tests |
Collaborator
Author
|
Builds were interrupted by a Jenkin upgrade. run tests |
Collaborator
Author
|
run tests |
alliepiper
force-pushed
the
if_target
branch
from
b869c2c to
5a87579
Compare
Collaborator
Author
|
run tests |
miscco
reviewed
May 2, 2023
|
|
||
| if (d_temp_storage == NULL) | ||
| { | ||
| if (d_temp_storage == nullptr) |
Collaborator
There was a problem hiding this comment.
Suggested change
| if (d_temp_storage == nullptr) | |
| if (!d_temp_storage) |
|
|
||
| reduce_agent ra(reduce_plan, num_items, stream, vshmem_ptr, "reduce_agent: single_tile only", debug_sync); | ||
| ra.launch(input_it, output_it, num_items, reduction_op); | ||
| char *vshmem_ptr = vshmem_size > 0 |
Collaborator
There was a problem hiding this comment.
Above you have char *const. We consolidate all uses
|
|
||
| size_t vshmem_storage = core::vshmem_size(partition_plan.shared_memory_size, | ||
| num_tiles); | ||
| if (!d_temp_storage) |
Collaborator
There was a problem hiding this comment.
No Change requested:
There is a whole lot of duplication with respect to the partitioning of the work. I am wondering whether we can extract that common functionality into a common helper function.
Not in this PR though
| for (size_t i = 0; i < n; i++) | ||
| { | ||
| // XXX Use proper random number generation facility. | ||
| h_keys[i] = FixedVector<T, N>(rand()); |
Collaborator
There was a problem hiding this comment.
Do we want to take the opportunity and use something from <random>
| static const cub::CacheLoadModifier LOAD_MODIFIER = _LOAD_MODIFIER; | ||
| static const cub::BlockScanAlgorithm SCAN_ALGORITHM = _SCAN_ALGORITHM; | ||
| }; // struct PtxPolicy | ||
| static constexpr int BLOCK_THREADS = _BLOCK_THREADS; |
Collaborator
There was a problem hiding this comment.
I would rather rename the template arguments than store additional static variables
| CUB_MIN(NOMINAL_4B_ITEMS_PER_THREAD, | ||
| CUB_MAX(1, (NOMINAL_4B_ITEMS_PER_THREAD * 4 / sizeof(T)))); | ||
|
|
||
| using Policy = PtxPolicy<128, |
Collaborator
There was a problem hiding this comment.
Can we give the magic 128 a name?
Collaborator
|
@allisonvacanti and I talked and we agreed to close this PR for now as this work has been de-prioritized. We will pick it back up if/when it becomes a priority again. |
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
No description provided.