Skip to content
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

Apply sentinel namespace to other map structures #163

Merged
merged 3 commits into from
May 27, 2022
Merged

Conversation

m3g4d1v3r
Copy link
Contributor

@m3g4d1v3r m3g4d1v3r commented May 26, 2022

Closes #162

Hello there!

This is my first contribution to a open source project, so please bear with me if I make a mistake or two.
I've tried to compile the last changes on my linux machine but unfortunately I've reached some errors at the benchmark compiling step.

Consolidate compiler generated dependencies of target DYNAMIC_MAP_BENCH
[ 43%] Building CUDA object benchmarks/CMakeFiles/DYNAMIC_MAP_BENCH.dir/hash_table/dynamic_map_bench.cu.o
synchronization.hpp(91): error: namespace "std" has no member "runtime_error"

Regarding the commits, I've managed to find two structures that the sentinel namespace could be applied. Please let me know if there are other structures that I've missed or if there were errors in my commits.

Best regards,
Marlon

@GPUtester
Copy link

Can one of the admins verify this patch?

@m3g4d1v3r m3g4d1v3r changed the title [WIP] Attempt to apply sentinel namespace to other map structures #162 [WIP] Attempt to apply sentinel namespace to other map structures May 26, 2022
@PointKernel PointKernel added the type: feature request New feature request label May 26, 2022
@PointKernel
Copy link
Member

@m3g4d1v3r Thanks for the contribution!

Consolidate compiler generated dependencies of target DYNAMIC_MAP_BENCH
[ 43%] Building CUDA object benchmarks/CMakeFiles/DYNAMIC_MAP_BENCH.dir/hash_table/dynamic_map_bench.cu.o
synchronization.hpp(91): error: namespace "std" has no member "runtime_error"

Which version of host compiler are you using? Include <stdexcept> in benchmarks/synchronization.hpp should fix the issue.

@PointKernel PointKernel added topic: static_multimap Issue related to the static_multimap topic: dynamic_map Issue related to the dynamic_map labels May 26, 2022
@ajschmidt8
Copy link
Member

ok to test

@m3g4d1v3r
Copy link
Contributor Author

Hello @PointKernel and @ajschmidt8 .

The inclusion of stdexcept library helped the compilation to pass the mentioned step. However, another error ocurred:

[ 55%] Building CUDA object _deps/nvbench-build/nvbench/CMakeFiles/nvbench.dir/blocking_kernel.cu.o
gcc-10: error: unrecognized command-line option ‘-Wunused-local-typedef’; did you mean ‘-Wunused-local-typedefs’?
gcc-10: error: unrecognized command-line option ‘-Wgnu’

I've used the following command to prepare the build environment:

export CC=clang && export CXX=clang++ && cmake -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ ..

@jrhemstad
Copy link
Collaborator

Thanks @m3g4d1v3r!

It looks like you're trying to compile with clang. We don't explicitly support this, though it is something we're looking into: #128

@m3g4d1v3r
Copy link
Contributor Author

I see @jrhemstad!

I guess I was confused about which compiler to use, so I ended up trying to compile with clang and supposed that I was missing some configuration in the cmake step.

Following your suggestions, I've successfully compiled the project the pull request diff using gcc version 10.2.1 for Debian. As for nvcc 11+ I've used nvidia-cuda-toolkit package, also available on Debian's distro.

@PointKernel PointKernel requested review from jrhemstad May 26, 2022 20:11
@PointKernel PointKernel added Needs Review Awaiting reviews before merging labels May 26, 2022
@PointKernel PointKernel changed the title [WIP] Attempt to apply sentinel namespace to other map structures Apply sentinel namespace to other map structures May 26, 2022
Copy link
Member

@PointKernel PointKernel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some small suggestions then it should be good to go.

@@ -52,7 +52,7 @@ static_multimap<Key, Value, Scope, Allocator, ProbeSequence>::static_multimap(
auto const grid_size = (get_capacity() + stride * block_size - 1) / (stride * block_size);

detail::initialize<atomic_key_type, atomic_mapped_type><<<grid_size, block_size, 0, stream>>>(
slots_.get(), empty_key_sentinel, empty_value_sentinel, get_capacity());
slots_.get(), empty_key_sentinel_, empty_value_sentinel_, get_capacity());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

include/cuco/static_multimap.cuh Outdated Show resolved Hide resolved
include/cuco/static_multimap.cuh Outdated Show resolved Hide resolved
@jrhemstad
Copy link
Collaborator

Thanks again @m3g4d1v3r

@jrhemstad jrhemstad merged commit b2af504 into NVIDIA:dev May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review Awaiting reviews before merging topic: dynamic_map Issue related to the dynamic_map topic: static_multimap Issue related to the static_multimap type: feature request New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Use sentinel namespace in all applicable map types
5 participants