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.

Sign up for GitHub

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

Silence spurious compiler warnings #3913

Merged
merged 4 commits into from
Oct 14, 2023
Merged

Silence spurious compiler warnings #3913

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
827bfa4
tweak the implementation to silence the spurious warning
seunghwak Oct 4, 2023
d39a4d9
add [[maybe_unused]]
seunghwak Oct 4, 2023
7585877
Merge branch 'branch-23.12' of github.com:rapidsai/cugraph into enh_f…
seunghwak Oct 12, 2023
1f34115
Merge branch 'branch-23.12' into enh_fix_warning
ChuckHastings Oct 14, 2023
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
28 changes: 12 additions & 16 deletions cpp/src/prims/per_v_transform_reduce_incoming_outgoing_e.cuh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,24 +513,20 @@ void per_v_transform_reduce_e(raft::handle_t const& handle,

static_assert(is_arithmetic_or_thrust_tuple_of_arithmetic<T>::value);

[[maybe_unused]] std::conditional_t<GraphViewType::is_storage_transposed,
edge_src_property_t<GraphViewType, T>,
edge_dst_property_t<GraphViewType, T>>
minor_tmp_buffer(handle); // relevant only when (GraphViewType::is_multi_gpu && !update_major
using minor_tmp_buffer_type = std::conditional_t<GraphViewType::is_storage_transposed,
edge_src_property_t<GraphViewType, T>,
edge_dst_property_t<GraphViewType, T>>;
std::unique_ptr<minor_tmp_buffer_type> minor_tmp_buffer{};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would an explicit std::optional<minor_tmp_buffer_type> defined here and initialized to std::nullopt also work? That's a code pattern we use quite a bit. Would that perhaps be a closer idiom to [[maybe_unused]] until the warning issue is corrected in the compiler?

Approving either way, just wondering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may work as well but std::optional is for run-time resolution. Here whether minor_tmp_buffer is used or not can be determined in compile-time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... but your comment reminded me that I didn't add [[maybe_unused]] in this update. Adding this to minor_tmp_buffer declaration.

if constexpr (GraphViewType::is_multi_gpu && !update_major) {
if constexpr (GraphViewType::is_storage_transposed) {
minor_tmp_buffer = edge_src_property_t<GraphViewType, T>(handle, graph_view);
} else {
minor_tmp_buffer = edge_dst_property_t<GraphViewType, T>(handle, graph_view);
}
minor_tmp_buffer = std::make_unique<minor_tmp_buffer_type>(handle, graph_view);
}

using edge_partition_minor_output_device_view_t =
std::conditional_t<update_major,
void /* dummy */,
std::conditional_t<GraphViewType::is_multi_gpu && !update_major,
detail::edge_partition_endpoint_property_device_view_t<
vertex_t,
decltype(minor_tmp_buffer.mutable_view().value_first())>>;
decltype(minor_tmp_buffer->mutable_view().value_first())>,
void /* dummy */>;

if constexpr (update_major) {
size_t partition_idx = 0;
Expand All @@ -549,7 +545,7 @@ void per_v_transform_reduce_e(raft::handle_t const& handle,
} else {
if constexpr (GraphViewType::is_multi_gpu) {
auto minor_init = init;
auto view = minor_tmp_buffer.view();
auto view = minor_tmp_buffer->view();
if (view.keys()) { // defer applying the initial value to the end as minor_tmp_buffer may not
// store values for the entire minor range
minor_init = ReduceOp::identity_element;
Expand All @@ -558,7 +554,7 @@ void per_v_transform_reduce_e(raft::handle_t const& handle,
auto const major_comm_rank = major_comm.get_rank();
minor_init = (major_comm_rank == 0) ? init : ReduceOp::identity_element;
}
fill_edge_minor_property(handle, graph_view, minor_init, minor_tmp_buffer.mutable_view());
fill_edge_minor_property(handle, graph_view, minor_init, minor_tmp_buffer->mutable_view());
} else {
thrust::fill(handle.get_thrust_policy(),
vertex_value_output_first,
Expand Down Expand Up @@ -699,7 +695,7 @@ void per_v_transform_reduce_e(raft::handle_t const& handle,
if constexpr (update_major) {
output_buffer = major_buffer_first;
} else {
output_buffer = edge_partition_minor_output_device_view_t(minor_tmp_buffer.mutable_view());
output_buffer = edge_partition_minor_output_device_view_t(minor_tmp_buffer->mutable_view());
}
} else {
output_buffer = vertex_value_output_first;
Expand Down Expand Up @@ -913,7 +909,7 @@ void per_v_transform_reduce_e(raft::handle_t const& handle,
auto const minor_comm_rank = minor_comm.get_rank();
auto const minor_comm_size = minor_comm.get_size();

auto view = minor_tmp_buffer.view();
auto view = minor_tmp_buffer->view();
if (view.keys()) { // applying the initial value is deferred to here
vertex_t max_vertex_partition_size{0};
for (int i = 0; i < major_comm_size; ++i) {
Expand Down
Loading