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

Make all headers self-contained #141

Merged
merged 10 commits into from
May 19, 2022

Conversation

PointKernel
Copy link
Member

@PointKernel PointKernel commented Mar 2, 2022

Closes #140

This PR reorders header groups in cuco and add missing headers to make sure all headers are self-contained.

To be merged after #155 and #156.

@PointKernel PointKernel added the type: bug Something isn't working label Mar 2, 2022
@PointKernel PointKernel mentioned this pull request Mar 14, 2022
@jrhemstad
Copy link
Collaborator

rerun tests

Copy link
Collaborator

@sleeepyjack sleeepyjack left a comment

Choose a reason for hiding this comment

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

Thanks @PointKernel for this PR! I made some comments targeting the automation of some reoccurring tasks regarding the code structure. This would also cause fewer diffs in future PRs. Happy to hear your thoughts on this.

benchmarks/hash_table/dynamic_map_bench.cu Show resolved Hide resolved
Comment on lines +17 to 24
#include <synchronization.hpp>

#include <cuco/dynamic_map.cuh>

#include <benchmark/benchmark.h>

#include <iostream>
#include <random>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be a permutation of the same includes. I think clang_format is already sorting these in lexicographic order for us if they are defined in a contiguous block, which is nice and consistent. However, lexicographic order might not always be correct or desired. Maybe we can customize the clang_format file to always produce an ordering that satisfies our needs and then just let the CI take care of it.

Copy link
Member Author

@PointKernel PointKernel May 18, 2022

Choose a reason for hiding this comment

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

The idea here is to separate different header groups and order them from "near" to "far". e.g. synchronization.hpp is a bench-local header thus it's placed before the library header cuco/dynamic_map.cuh. gbench header is even further but considered closer than std headers.

This grouping method seems a bit awkward with only one file in each group but will show its advantage with more headers involved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok, this makes sense. I wonder however if we should add a CI script for that. Basically extract all includes, check in the include tree where this file originates, group and then reorder.

benchmarks/hash_table/static_map_bench.cu Outdated Show resolved Hide resolved
include/cuco/detail/dynamic_map_kernels.cuh Outdated Show resolved Hide resolved
#include <cuco/detail/pair.cuh>
#include <cub/cub.cuh>

#include <cuda/std/atomic>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We access the atomic types through a typedef atomicT, so we don't need to include <cuda/std/atomic> here... At least I think that's the case.

Copy link
Member Author

@PointKernel PointKernel May 18, 2022

Choose a reason for hiding this comment

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

It's still needed due to atomic operations like this:

num_matches->fetch_add(block_num_matches, cuda::std::memory_order_relaxed);

Removing it will cause build failure:

/home/yunsongw/Work/cuCollections/include/cuco/detail/static_multimap/kernels.cuh(256): error: name followed by "::" must be a class or namespace name
          detected during instantiation of "std::size_t cuco::static_multimap<Key, Value, Scope, Allocator, ProbeSequence>::count_outer(InputIt, InputIt, cudaStream_t, KeyEqual) const [with Key=int, Value=int, Scope=cuda::std::__4::__detail::thread_scope_device, Allocator=cuco::cuda_allocator<char>, ProbeSequence=cuco::double_hashing<8U, cuco::detail::MurmurHash3_32<int>, cuco::detail::MurmurHash3_32<int>>, InputIt=thrust::detail::normal_iterator<thrust::device_ptr<int>>, KeyEqual=thrust::equal_to<int>]" 

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes but in L242 we call view.count() which is also not declared (only after the template is instantiated).

include/cuco/dynamic_map.cuh Show resolved Hide resolved
tests/dynamic_map/unique_sequence_test.cu Show resolved Hide resolved
tests/static_map/custom_type_test.cu Show resolved Hide resolved
Copy link
Collaborator

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

Thanks @PointKernel!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Headers are not completely self-contained
3 participants