-
Notifications
You must be signed in to change notification settings - Fork 915
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
Normalizing offsets iterator #14234
Merged
rapids-bot
merged 47 commits into
rapidsai:branch-23.12
from
davidwendt:offsets-iterator
Nov 13, 2023
Merged
Normalizing offsets iterator #14234
Changes from 41 commits
Commits
Show all changes
47 commits
Select commit
Hold shift + click to select a range
32e1029
Enable indexalator for device code
davidwendt 1548588
Merge branch 'branch-23.12' into indexalator-device-enable
davidwendt f6419b4
return ref experiment
davidwendt b5b4449
Merge branch 'branch-23.12' into indexalator-device-enable
davidwendt 0e369dd
Normalizing offsets iterator
davidwendt e451116
Merge branch 'branch-23.12' into indexalator-device-enable
davidwendt 7dcb134
23.12 baseline compile-time commit
davidwendt a248d75
Merge branch 'branch-23.12' into indexalator-device-enable
davidwendt 88f6dff
undo temp change
davidwendt 081cb84
use cudf::is_index_type
davidwendt a28a9ff
use cudf::is_index_type part 2
davidwendt df063b6
Merge branch 'branch-23.12' into indexalator-device-enable
davidwendt f5c898c
Merge branch 'indexalator-device-enable' into offsets-iterator
davidwendt ccc5bf5
add offsetalator factory
davidwendt 73c04d8
Merge branch 'branch-23.12' into indexalator-device-enable
davidwendt eb586f4
use size_t for index_sizeof_fn
davidwendt 4ba5a70
Merge branch 'indexalator-device-enable' into offsets-iterator
davidwendt e76ec97
Merge branch 'branch-23.12' into indexalator-device-enable
davidwendt 1ae36f3
Merge branch 'branch-23.12' into indexalator-device-enable
davidwendt 6fdeadf
Merge branch 'indexalator-device-enable' into offsets-iterator
davidwendt 84e7510
Merge branch 'branch-23.12' into offsets-iterator
davidwendt 2bfb3e1
Merge branch 'branch-23.12' into offsets-iterator
davidwendt 8d45877
Merge branch 'branch-23.12' into offsets-iterator
davidwendt 352f516
Merge branch 'branch-23.12' into offsets-iterator
davidwendt 1add402
fix exception message
davidwendt e9cfafe
Merge branch 'branch-23.12' into offsets-iterator
davidwendt e5f5589
Merge branch 'branch-23.12' into offsets-iterator
davidwendt 09afe63
Merge branch 'branch-23.12' into offsets-iterator
davidwendt 704c853
rework offsetalator/indexalator dispatch logic
davidwendt f45ee6d
Merge branch 'branch-23.12' into offsets-iterator
davidwendt a132e33
Merge branch 'offsets-iterator' of github.com:davidwendt/cudf into of…
davidwendt 8ee15a3
add alignas
davidwendt 43c4984
Merge branch 'offsets-iterator' of github.com:davidwendt/cudf into of…
davidwendt a97d061
add more alignases
davidwendt 1ed5e29
re-enable device test
davidwendt 5839fcd
remove unneeded test
davidwendt e0d4e5f
change std::enable_if_t to CUDF_ENABLE_IF
davidwendt 6ec6258
add anonymous namespace around internal functor
davidwendt 8514a71
Merge branch 'branch-23.12' into offsets-iterator
davidwendt 3a21fe1
remove type-dispatcher call from ctor
davidwendt ab0edb7
Merge branch 'branch-23.12' into offsets-iterator
davidwendt 7b10cb7
Merge branch 'branch-23.12' into offsets-iterator
davidwendt f4634f9
removed unneeded dispatch in factory
davidwendt 713d601
Merge branch 'branch-23.12' into offsets-iterator
davidwendt 0d8caf1
Merge branch 'branch-23.12' into offsets-iterator
davidwendt 56a73c5
fix doxygen comments
davidwendt 23a8bf6
Merge branch 'branch-23.12' into offsets-iterator
davidwendt 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
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.
Why is this not
T const* tp
?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.
Because that is not the type that is being passed to the function.
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.
So to understand the impact of
type_dispatcher
on the reworked design, it seems to me like we are still using it but there's no cascading calls totype_dispatcher
and it's only called exactly once. Is that correct?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. We only call the type-dispatcher in the factory now.
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, and once when setting up the non-templated class
input_indexelator::normalize_input
. If you use a normal if-else dispatch there instead oftype_dispatcher
, are you able to see any benefits? Especially insrc/reductions/scan/scan_inclusive.cu.o
where there's a 6 minute compile-time increaseThere 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.
Thanks! Looks good.
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.
But you are correct, the base class's type-dispatcher is still called inside every
element()
call.I think that is worth considering 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.
We'll have to study the assembly here. Is the
type_dispatcher
expanded only once when the class is compiled (so when the header is included) or is it expanded every timeelement()
is called?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 created a separate ctor that just passes the width instead of type-dispatching to resolve it.
This did improved the compile time: https://downloads.rapids.ai/ci/cudf/pull-request/14234/ab0edb7/cuda12_x86_64.ninja_log.html
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.
So we halved the compile time increment in
scan_inclusive
? That is good!