-
Notifications
You must be signed in to change notification settings - Fork 35
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
Adjustments in AlpakaCore/prefixScan and its test + Add helper functions to handle workdiv #167
Merged
+738
−402
Merged
Changes from 19 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
902ee9e
prefixScan: Keep same workload between CUDA and ALPAKA tests, to be a…
ghugo83 6c09567
Remove unsupported #pragma unroll. Remove unused variables. Cannot us…
ghugo83 0038c02
Only calculate warpPrefixScan workDiv for GPU case
ghugo83 bea0450
Cleaner to directly call with cms::alpakatools::
ghugo83 4a4ca88
testPrefixScan: take into account dependency on number of elements in…
ghugo83 06d0c82
IMPORTANT: Should not have alpaka::wait::wait(queue); in between kern…
ghugo83 805756b
Comments
ghugo83 99cb983
clang-format
ghugo83 9dc2943
[alpaka] Add CMS_UNROLL_LOOP macro written by fwyzard from https://gi…
ghugo83 d6d8c0a
Add helper functions to directly handle element indices in non-stride…
ghugo83 8fb9967
Fixed issues in helper
ghugo83 d1f739b
Also add possibility to offset the indices in the helper functions + …
ghugo83 72b7c47
[alpaka] All call sites are now using the helper functions
ghugo83 a1f88bc
[alpakatest] Call sites are now using the 1D helper functions. No add…
ghugo83 3a9481b
Simplify helper functions
ghugo83 2f2b54b
Added comments in helpers
ghugo83 9d0b0d5
Latest fix in helper + Propagate latest helper functions to all alpak…
ghugo83 1c98947
clang-format
ghugo83 48d4d15
Pass function by value in helper functions
ghugo83 36f8c3d
Pass lamdas by && in helper functions
ghugo83 b4c3682
Inlining these helper functions leads to better perf
ghugo83 7c64537
Revert "Inlining these helper functions leads to better perf"
ghugo83 89d69fe
Revert "Pass lamdas by && in helper functions"
ghugo83 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.
I think this should be either
const Func&
(as it was) orFunc&&
. It beingconst
prevents mutating functors (which I'd think to be fine), and taking it by value implies a copy (which is not needed).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.
Hmm the copies are eluded anyway?
Don't know, in practice, passing the lambda by value clearly leads to better performance than passing by
const &
, or passing by&&
and then forwarding.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.
Passing the lambda by && and then forwarding, and inlining these functions, seems to be a good compromise, will go for that.
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.
By quick look (that I should have done earlier),
std
and SYCL seem to take such functors by "non-const value", and Kokkos and Alpaka by "const reference". Just do double check, did you observe that passing the lambda by value gives better performance than reference?If I've understood correctly, calling
std::forward()
many times on the same object is not strictly speaking safe (has the same issue asstd::move()
), even if it may work in practice in many cases.Sorry for going back-and-forth, how about going back to the by-value (but without
const
), or keep the universal reference and remove thestd::forward
?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.
Re-looking better at this, I do not see clear diff in perf.
By value still makes the assumption that the function is light, which should be the case most of the time, but is not that nice.
Func&& without forwarding loses a bit of interest, as the parameter (even a temporary) would be bound to an lvalue, no?
Bah maybe this is too much hair-pulling
I think I could revert to the initial implementation with
const&
?Also, as this does not change the helper functions interface anyway, maybe we could accept this, move on, and re-evaluate later when the full track reconstruction has been ported to have a better metric on the potential impact on perf?
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 agree, although that is the precedent that
std
sets (not that we should always agree withstd
).Within the function body the
func
ofFunc&& func
would indeed be an lvalue. But the function argument itself would bind to anything without requiring the thefunc
to be const.I agree, we should move on. I'm going to run tests for this and merge then. (although the initial implementation was/is
const Func
instead ofconst Func&
)