Use memset in deep_copy #3944
Merged
Use memset in deep_copy #3944
Conversation
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
|
Some more results on V100 |
|
SYCL V100 before after |
|
For HIP I am seeing (old) and (new) |
|
Please use the benchmark comparison tool when you post results |
masterleinad
force-pushed
the
cuda_deep_copy_memset
branch
from
4cd659a
to
948958c
Compare
masterleinad
force-pushed
the
cuda_deep_copy_memset
branch
from
9ff4137
to
5e7766b
Compare
|
CUDA: |
|
SYCL: |
|
HIP |
|
I think this is now good enough to be looked at and being discussed (in particular if the results are good enough). |
masterleinad
changed the title
|
I couldn't find any |
|
The testing script was #include <Kokkos_Core.hpp>
#include <benchmark/benchmark.h>
struct TestZeroing
{
template <class ExecutionSpace, class View>
TestZeroing(ExecutionSpace const &s, View x) {
run(s, x);
}
template <class ExecutionSpace, class View>
void run(ExecutionSpace const &, View x) {
Kokkos::deep_copy(x, 0);
}
};
struct TestZeroingAsync
{
template <class ExecutionSpace, class View>
TestZeroingAsync(ExecutionSpace const &s, View x) {
run(s, x);
}
template <class ExecutionSpace, class View>
void run(ExecutionSpace const &s, View x) {
Kokkos::deep_copy(s, x, 0);
s.fence();
}
};
struct TestOne
{
template <class ExecutionSpace, class View>
TestOne(ExecutionSpace const &s, View x) {
run(s, x);
}
template <class ExecutionSpace, class View>
void run(ExecutionSpace const &, View x) {
Kokkos::deep_copy(x, 1);
}
};
struct TestOneAsync
{
template <class ExecutionSpace, class View>
TestOneAsync(ExecutionSpace const &s, View x) {
run(s, x);
}
template <class ExecutionSpace, class View>
void run(ExecutionSpace const &s, View x) {
Kokkos::deep_copy(s, x, 1);
s.fence();
}
};
template <class K, typename T>
void BM_generic(benchmark::State &state) {
#if defined(KOKKOS_ENABLE_HIP)
using ExecutionSpace = Kokkos::Experimental::HIP;
#elif defined(KOKKOS_ENABLE_CUDA)
using ExecutionSpace = Kokkos::Cuda;
#elif defined(KOKKOS_ENABLE_OPENMPTARGET)
using ExecutionSpace = Kokkos::Experimental::OpenMPTarget;
#else
using ExecutionSpace = Kokkos::Experimental::SYCL;
#endif
int n = state.range(0);
ExecutionSpace space{};
Kokkos::View<T *, ExecutionSpace> x("x", n);
K(space, x); // warm-up
for (auto _ : state) {
K(space, x);
}
state.counters["Bandwidth"] =
benchmark::Counter(sizeof(T) * n,
benchmark::Counter::kIsIterationInvariantRate);
}
#define REGISTER_BENCHMARK(KERNEL, TYPE) \
BENCHMARK_TEMPLATE(BM_generic, KERNEL, TYPE) \
->RangeMultiplier(8) \
->Range(1024, 8 << 24) \
->UseRealTime();
REGISTER_BENCHMARK(TestZeroing, char);
REGISTER_BENCHMARK(TestZeroing, int);
REGISTER_BENCHMARK(TestZeroing, double);
REGISTER_BENCHMARK(TestZeroing, std::complex<double>);
REGISTER_BENCHMARK(TestZeroingAsync, char);
REGISTER_BENCHMARK(TestZeroingAsync, int);
REGISTER_BENCHMARK(TestZeroingAsync, double);
REGISTER_BENCHMARK(TestZeroingAsync, std::complex<double>);
REGISTER_BENCHMARK(TestOne, char);
REGISTER_BENCHMARK(TestOne, int);
REGISTER_BENCHMARK(TestOne, double);
REGISTER_BENCHMARK(TestOne, std::complex<double>);
REGISTER_BENCHMARK(TestOneAsync, char);
REGISTER_BENCHMARK(TestOneAsync, int);
REGISTER_BENCHMARK(TestOneAsync, double);
REGISTER_BENCHMARK(TestOneAsync, std::complex<double>);
int main(int argc, char **argv) {
Kokkos::initialize(argc, argv);
benchmark::Initialize(&argc, argv);
benchmark::RunSpecifiedBenchmarks();
Kokkos::finalize();
} |
|
Hm Looks like for HIP right now its not a win generally. |
|
Still probably good enough |
crtrott
approved these changes
Apr 16, 2021
dalg24
requested changes
Apr 22, 2021
core/src/Kokkos_CopyViews.hpp
Outdated
| } | ||
|
|
||
| template <typename ExecutionSpace, class DT, class... DP> | ||
| inline void plain_memcpy( |
There was a problem hiding this comment.
"Plain memcpy" does not describe what this function does.
Suggested change
| inline void plain_memcpy( | |
| void copy_assign_value( |
(must find a more appropriate name)
There was a problem hiding this comment.
The point I tried making here is that we know that the buffer to be filled is contiguous in memory. What about contiguous_fill or similar?
There was a problem hiding this comment.
... or flat_fill since we are calling ViewFill<ViewTypeFlat,... in the end.
There was a problem hiding this comment.
contiguous_fill sounds good to me.
core/src/Kokkos_CopyViews.hpp
Outdated
| } | ||
|
|
||
| template <class DT, class... DP> | ||
| inline void memset(const View<DT, DP...>& dst, |
There was a problem hiding this comment.
Suggested change
| inline void memset(const View<DT, DP...>& dst, | |
| void fill(const View<DT, DP...>& dst, |
(must fix)
There was a problem hiding this comment.
I think the choice here depends a little bit on the one for plain_memcpy. This function does essentially the same (and calls plain_memcpy but has a special case for the zero_byte_case).
I don't like fill since it doesn't say that the underlying memory must be contiguous. memset would say that but is too strong.
What about flat_fill_or_memset here?
|
Results for are pretty impressive. |
masterleinad
force-pushed
the
cuda_deep_copy_memset
branch
from
9f4f71b
to
7a70acc
Compare
masterleinad
force-pushed
the
cuda_deep_copy_memset
branch
from
7a70acc
to
2fb4409
Compare
|
@dalg24 I addressed your commits apart from the naming ones where I'd like to discuss some more and also used |
masterleinad
force-pushed
the
cuda_deep_copy_memset
branch
from
bce5577
to
b2f0413
Compare
|
I also looked into doing the but it's not quite clear if that's better (or what a good threshold would be) than a single |
|
|
crtrott
requested changes
Apr 26, 2021
Yeah thats fine. |
|
I replaced |
crtrott
approved these changes
Apr 27, 2021
dalg24
reviewed
Apr 27, 2021
|
Cleanup history or want me to squash and merge? |
|
Just squash and merge. I don't think there is anything worth keeping in the history. |
dalg24
approved these changes
Apr 28, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #3930. As noted in #3930 (comment) this seems to be advantageous at least for
Cudabut we should benchmark this a little better and also possibly extend to other backends.