Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,11 @@ if(RMM_NVTX)
target_compile_definitions(rmm PUBLIC RMM_NVTX)
endif()

set(RMM_CXX_FLAGS -Wall -Werror -Wextra -Wsign-conversion -Wno-unknown-pragmas
set(RMM_CXX_FLAGS -Wall -Werror -Wextra -Wshadow -Wsign-conversion -Wno-unknown-pragmas
-Wno-error=deprecated-declarations)
set(RMM_CUDA_FLAGS
-Werror=all-warnings
-Xcompiler=-Wall,-Werror,-Wextra,-Wsign-conversion,-Wno-error=deprecated-declarations)
-Xcompiler=-Wall,-Werror,-Wextra,-Wshadow,-Wsign-conversion,-Wno-error=deprecated-declarations)
target_compile_options(rmm PRIVATE "$<$<COMPILE_LANGUAGE:CXX>:${RMM_CXX_FLAGS}>"
"$<$<COMPILE_LANGUAGE:CUDA>:${RMM_CUDA_FLAGS}>")

Expand Down
4 changes: 2 additions & 2 deletions cpp/benchmarks/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
option(DISABLE_DEPRECATION_WARNING "Disable warnings generated from deprecated declarations." OFF)
option(PER_THREAD_DEFAULT_STREAM "Build with per-thread default stream" OFF)

set(RMM_BENCHMARKS_CXX_FLAGS -Wall -Werror -Wextra -Wsign-conversion -Wno-unknown-pragmas)
set(RMM_BENCHMARKS_CXX_FLAGS -Wall -Werror -Wextra -Wshadow -Wsign-conversion -Wno-unknown-pragmas)
set(RMM_BENCHMARKS_CUDA_FLAGS -Werror=all-warnings
-Xcompiler=-Wall,-Werror,-Wextra,-Wsign-conversion)
-Xcompiler=-Wall,-Werror,-Wextra,-Wshadow,-Wsign-conversion)

if(PER_THREAD_DEFAULT_STREAM)
message(STATUS "RMM: Building benchmarks with per-thread default stream")
Expand Down
36 changes: 18 additions & 18 deletions cpp/benchmarks/random_allocations/random_allocations.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ constexpr std::size_t size_mb{1 << 20};
struct allocation {
void* ptr{nullptr};
std::size_t size{0};
allocation(void* ptr, std::size_t size) : ptr{ptr}, size{size} {}
allocation(void* pointer, std::size_t bytes) : ptr{pointer}, size{bytes} {}
allocation() = default;
};

Expand Down Expand Up @@ -219,20 +219,20 @@ static void num_size_range(benchmark::Benchmark* bench)
}
}

int num_allocations = -1; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
int max_size = -1; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
int g_num_allocations = -1; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
int g_max_size = -1; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)

void benchmark_range(benchmark::Benchmark* bench)
{
if (num_allocations > 0) {
if (max_size > 0) {
bench->Args({num_allocations, max_size})->Unit(benchmark::kMillisecond);
if (g_num_allocations > 0) {
if (g_max_size > 0) {
bench->Args({g_num_allocations, g_max_size})->Unit(benchmark::kMillisecond);
} else {
size_range(bench, num_allocations);
size_range(bench, g_num_allocations);
}
} else {
if (max_size > 0) {
num_range(bench, max_size);
if (g_max_size > 0) {
num_range(bench, g_max_size);
} else {
num_size_range(bench);
}
Expand Down Expand Up @@ -297,9 +297,9 @@ int main(int argc, char** argv)
"Maximum allocation size (default of 0 tests a range)",
cxxopts::value<int>()->default_value("4096"));

auto args = options.parse(argc, argv);
num_allocations = args["numallocs"].as<int>();
max_size = args["maxsize"].as<int>();
auto args = options.parse(argc, argv);
g_num_allocations = args["numallocs"].as<int>();
g_max_size = args["maxsize"].as<int>();

if (args.count("profile") > 0) {
std::map<std::string, MRFactoryFunc> const funcs({{"arena", &make_arena},
Expand All @@ -309,20 +309,20 @@ int main(int argc, char** argv)
{"pool", &make_pool}});
auto resource = args["resource"].as<std::string>();

std::cout << "Profiling " << resource << " with " << num_allocations << " allocations of max "
<< max_size << "B\n";
std::cout << "Profiling " << resource << " with " << g_num_allocations
<< " allocations of max " << g_max_size << "B\n";

profile_random_allocations(funcs.at(resource),
static_cast<std::size_t>(num_allocations),
static_cast<std::size_t>(max_size));
static_cast<std::size_t>(g_num_allocations),
static_cast<std::size_t>(g_max_size));

std::cout << "Finished\n";
} else {
if (args.count("numallocs") == 0) { // if zero reset to -1 so we benchmark over a range
num_allocations = -1;
g_num_allocations = -1;
}
if (args.count("maxsize") == 0) { // if zero reset to -1 so we benchmark over a range
max_size = -1;
g_max_size = -1;
}

if (args.count("resource") > 0) {
Expand Down
8 changes: 4 additions & 4 deletions cpp/benchmarks/replay/replay.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ using MRFactoryFunc = std::function<any_device_resource(std::size_t)>;
struct allocation {
allocation() = default;
void* ptr{};
allocation(void* ptr, std::size_t size) : ptr{ptr}, size{size} {}
allocation(void* pointer, std::size_t bytes) : ptr{pointer}, size{bytes} {}
std::size_t size{};
};

Expand Down Expand Up @@ -359,14 +359,14 @@ int main(int argc, char** argv)
"Enable verbose printing of log events",
cxxopts::value<bool>()->default_value("false"));

auto args = options.parse(argc, argv);
auto parsed_args = options.parse(argc, argv);

if (args.count("file") == 0) {
if (parsed_args.count("file") == 0) {
std::cout << options.help() << std::endl;
exit(0);
}

return args;
return parsed_args;
}();
Comment on lines -362 to 370

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why?


auto filename = args["file"].as<std::string>();
Expand Down
4 changes: 2 additions & 2 deletions cpp/benchmarks/synchronization/synchronization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
cuda_event_timer::cuda_event_timer(benchmark::State& state,
bool flush_l2_cache,
rmm::cuda_stream_view stream)
: stream(stream), p_state(&state)
: stream_(stream), p_state(&state)
{
Comment on lines -23 to 24

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW, as is the case with most style guides, this is tremendously hateful.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@wence- Can you clarify what you mean here? Are you saying you dislike the idea of suffixing member variables with an underscore as a general practice? I don't mind that rule, and I find it somewhat helpful for reasoning about member state.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am ok with us deciding that all struct members should have (say) trailing underscore names.

But doing single point rewrites to pacify a warning that I broadly think is just not worth it makes the code worse.

I note as well that the ctor body here is no longer implemented correctly (it refers to stream, not stream_) so this warning didn't help us anyway

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, that is helpful. I agree this will need some polish and close verification to get right.

// flush all of L2$
if (flush_l2_cache) {
Expand All @@ -47,7 +47,7 @@ cuda_event_timer::cuda_event_timer(benchmark::State& state,

cuda_event_timer::~cuda_event_timer()
{
RMM_CUDA_ASSERT_OK(cudaEventRecord(stop, stream.value()));
RMM_CUDA_ASSERT_OK(cudaEventRecord(stop, stream_.value()));
RMM_CUDA_ASSERT_OK(cudaEventSynchronize(stop));

float milliseconds = 0.0F;
Expand Down
2 changes: 1 addition & 1 deletion cpp/benchmarks/synchronization/synchronization.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,6 @@ class cuda_event_timer {
private:
cudaEvent_t start{};
cudaEvent_t stop{};
rmm::cuda_stream_view stream{};
rmm::cuda_stream_view stream_{};
benchmark::State* p_state{};
};
34 changes: 23 additions & 11 deletions cpp/benchmarks/utilities/log_parser.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,29 +34,41 @@ struct event {
event(event&&) noexcept = default;
event& operator=(event&&) noexcept = default;
~event() = default;
event(action act, std::size_t size, void const* ptr)
event(action action_type, std::size_t bytes, void const* ptr)
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
: act{act}, size{size}, pointer{reinterpret_cast<uintptr_t>(ptr)}
: act{action_type}, size{bytes}, pointer{reinterpret_cast<uintptr_t>(ptr)}
{
}

// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
event(action act, std::size_t size, uintptr_t ptr) : act{act}, size{size}, pointer{ptr} {}
event(action action_type, std::size_t bytes, uintptr_t ptr)
: act{action_type}, size{bytes}, pointer{ptr}
{
}

event(std::size_t tid,
action act,
std::size_t size, // NOLINT(bugprone-easily-swappable-parameters)
action action_type,
std::size_t bytes, // NOLINT(bugprone-easily-swappable-parameters)
uintptr_t ptr,
uintptr_t stream,
std::size_t index)
: act{act}, size{size}, pointer{ptr}, thread_id{tid}, stream{stream}, index{index}
uintptr_t stream_id,
std::size_t event_index)
: act{action_type},
size{bytes},
pointer{ptr},
thread_id{tid},
stream{stream_id},
index{event_index}
{
}

event(
std::size_t tid, action act, std::size_t size, void* ptr, uintptr_t stream, std::size_t index)
event(std::size_t tid,
action action_type,
std::size_t bytes,
void* ptr,
uintptr_t stream_id,
std::size_t event_index)
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
: event{tid, act, size, reinterpret_cast<uintptr_t>(ptr), stream, index}
: event{tid, action_type, bytes, reinterpret_cast<uintptr_t>(ptr), stream_id, event_index}
{
}

Expand Down
4 changes: 2 additions & 2 deletions cpp/include/rmm/cuda_stream.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,11 @@ class cuda_stream {
/**
* @brief Construct a new CUDA stream object
*
* @param flags Stream creation flags.
* @param creation_flags Stream creation flags.
*
* @throw rmm::cuda_error if stream creation fails
*/
cuda_stream(cuda_stream::flags flags = cuda_stream::flags::sync_default);
cuda_stream(cuda_stream::flags creation_flags = cuda_stream::flags::sync_default);

/**
* @brief Returns true if the owned stream is non-null
Expand Down
8 changes: 4 additions & 4 deletions cpp/include/rmm/exec_policy.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,11 @@ class exec_policy : public thrust_exec_policy_t {
/**
* @brief Construct a new execution policy object
*
* @param stream The stream on which to allocate temporary memory
* @param stream_view The stream on which to allocate temporary memory
* @param mr The resource to use for allocating temporary memory
*/
explicit exec_policy(
cuda_stream_view stream = cuda_stream_default,
cuda_stream_view stream_view = cuda_stream_default,
cuda::mr::any_resource<cuda::mr::device_accessible> mr = mr::get_current_device_resource_ref());
};

Expand All @@ -68,11 +68,11 @@ class exec_policy_nosync : public thrust_exec_policy_nosync_t {
/**
* @brief Construct a new execution policy object
*
* @param stream The stream on which to allocate temporary memory
* @param stream_view The stream on which to allocate temporary memory
* @param mr The resource to use for allocating temporary memory
*/
explicit exec_policy_nosync(
cuda_stream_view stream = cuda_stream_default,
cuda_stream_view stream_view = cuda_stream_default,
cuda::mr::any_resource<cuda::mr::device_accessible> mr = mr::get_current_device_resource_ref());
};

Expand Down
4 changes: 2 additions & 2 deletions cpp/include/rmm/mr/detail/coalescing_free_list.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ namespace mr::detail {
*/
struct block : public block_base {
block() = default;
block(char* ptr, std::size_t size, bool is_head)
: block_base{ptr}, size_bytes{size}, head{is_head}
block(char* raw_pointer, std::size_t size, bool is_head)
: block_base{raw_pointer}, size_bytes{size}, head{is_head}
{
Comment on lines -30 to 32

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why?

}

Expand Down
2 changes: 1 addition & 1 deletion cpp/include/rmm/mr/detail/free_list.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ struct block_base {
void* ptr{}; ///< Raw memory pointer

block_base() = default;
block_base(void* ptr) : ptr{ptr} {};
block_base(void* raw_pointer) : ptr{raw_pointer} {};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why?


/// Returns the raw pointer for this block
[[nodiscard]] inline void* pointer() const { return ptr; }
Expand Down
4 changes: 2 additions & 2 deletions cpp/include/rmm/mr/pinned_host_memory_resource.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ class pinned_host_memory_resource final {
[[maybe_unused]] std::size_t alignment = rmm::CUDA_ALLOCATION_ALIGNMENT) noexcept
{
std::size_t constexpr alloc_alignment = rmm::CUDA_ALLOCATION_ALIGNMENT;
rmm::detail::aligned_host_deallocate(ptr, bytes, alloc_alignment, [](void* ptr) {
RMM_ASSERT_CUDA_SUCCESS_SAFE_SHUTDOWN(cudaFreeHost(ptr));
rmm::detail::aligned_host_deallocate(ptr, bytes, alloc_alignment, [](void* memory) {
RMM_ASSERT_CUDA_SUCCESS_SAFE_SHUTDOWN(cudaFreeHost(memory));
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, I see, -Wshadow is fundamentally broken. This lambda captures no closure but somehow the compiler warns that the name shadows a local.

}

Expand Down
2 changes: 1 addition & 1 deletion cpp/include/rmm/mr/system_memory_resource.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ class system_memory_resource final {
RMM_ASSERT_CUDA_SUCCESS_SAFE_SHUTDOWN(cudaStreamSynchronize(stream.get()));

rmm::detail::aligned_host_deallocate(
ptr, bytes, rmm::CUDA_ALLOCATION_ALIGNMENT, [](void* ptr) { ::operator delete(ptr); });
ptr, bytes, rmm::CUDA_ALLOCATION_ALIGNMENT, [](void* memory) { ::operator delete(memory); });
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why?


/**
Expand Down
6 changes: 3 additions & 3 deletions cpp/src/cuda_stream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@

namespace rmm {

cuda_stream::cuda_stream(cuda_stream::flags flags)
: stream_{[flags]() {
cuda_stream::cuda_stream(cuda_stream::flags creation_flags)
: stream_{[creation_flags]() {
auto* stream = new cudaStream_t; // NOLINT(cppcoreguidelines-owning-memory)
// TODO: use std::to_underlying once C++23 is allowed.
RMM_CUDA_TRY(cudaStreamCreateWithFlags(
stream, static_cast<std::underlying_type_t<cuda_stream::flags>>(flags)));
stream, static_cast<std::underlying_type_t<cuda_stream::flags>>(creation_flags)));
return stream;
}(),
[](cudaStream_t* stream) {
Expand Down
12 changes: 6 additions & 6 deletions cpp/src/exec_policy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,18 @@

namespace rmm {

exec_policy::exec_policy(cuda_stream_view stream,
exec_policy::exec_policy(cuda_stream_view stream_view,
cuda::mr::any_resource<cuda::mr::device_accessible> mr)
: thrust_exec_policy_t(
thrust::cuda::par(mr::thrust_allocator<char>(stream, std::move(mr))).on(stream.value()))
: thrust_exec_policy_t(thrust::cuda::par(mr::thrust_allocator<char>(stream_view, std::move(mr)))
.on(stream_view.value()))
{
}

exec_policy_nosync::exec_policy_nosync(cuda_stream_view stream,
exec_policy_nosync::exec_policy_nosync(cuda_stream_view stream_view,
cuda::mr::any_resource<cuda::mr::device_accessible> mr)
: thrust_exec_policy_nosync_t(
thrust::cuda::par_nosync(mr::thrust_allocator<char>(stream, std::move(mr)))
.on(stream.value()))
thrust::cuda::par_nosync(mr::thrust_allocator<char>(stream_view, std::move(mr)))
.on(stream_view.value()))
{
}

Expand Down
6 changes: 3 additions & 3 deletions cpp/src/logger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ std::string default_pattern() { return "[%6t][%H:%M:%S:%f][%-6l] %v"; }
rapids_logger::logger& default_logger()
{
static rapids_logger::logger logger_ = [] {
rapids_logger::logger logger_{"RMM", {default_sink()}};
logger_.set_pattern(default_pattern());
return logger_;
rapids_logger::logger instance{"RMM", {default_sink()}};
instance.set_pattern(default_pattern());
return instance;
}();
return logger_;
}
Expand Down
10 changes: 5 additions & 5 deletions cpp/src/mr/detail/arena_memory_resource_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,18 @@ void* arena_memory_resource_impl::allocate(cuda::stream_ref stream,
#else
bytes = rmm::align_up(bytes, rmm::CUDA_ALLOCATION_ALIGNMENT);
#endif
auto& arena = get_arena(sv);
auto& stream_arena = get_arena(sv);

{
std::shared_lock lock(mtx_);
void* pointer = arena.allocate_sync(bytes);
void* pointer = stream_arena.allocate_sync(bytes);
if (pointer != nullptr) { return pointer; }
}

{
std::unique_lock lock(mtx_);
defragment();
void* pointer = arena.allocate_sync(bytes);
void* pointer = stream_arena.allocate_sync(bytes);
if (pointer == nullptr) {
if (dump_log_on_failure_) { dump_memory_log(bytes); }
auto const msg = std::string("Maximum pool size exceeded (failed to allocate ") +
Expand All @@ -73,11 +73,11 @@ void arena_memory_resource_impl::deallocate(cuda::stream_ref stream,
#else
bytes = rmm::align_up(bytes, rmm::CUDA_ALLOCATION_ALIGNMENT);
#endif
auto& arena = get_arena(sv);
auto& stream_arena = get_arena(sv);

{
std::shared_lock lock(mtx_);
if (arena.deallocate(sv, ptr, bytes)) { return; }
if (stream_arena.deallocate(sv, ptr, bytes)) { return; }
}

{
Expand Down
5 changes: 3 additions & 2 deletions cpp/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@
option(DISABLE_DEPRECATION_WARNING "Disable warnings generated from deprecated declarations." OFF)
option(CODE_COVERAGE "Enable generating code coverage with gcov." OFF)

set(RMM_TESTS_CXX_FLAGS -Wall -Werror -Wextra -Wsign-conversion -Wno-unknown-pragmas)
set(RMM_TESTS_CUDA_FLAGS -Werror=all-warnings -Xcompiler=-Wall,-Werror,-Wextra,-Wsign-conversion)
set(RMM_TESTS_CXX_FLAGS -Wall -Werror -Wextra -Wshadow -Wsign-conversion -Wno-unknown-pragmas)
set(RMM_TESTS_CUDA_FLAGS -Werror=all-warnings
-Xcompiler=-Wall,-Werror,-Wextra,-Wshadow,-Wsign-conversion)

include(rapids-test)
rapids_test_init()
Expand Down
Loading
Loading