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

Add CUDF_UNREACHABLE macro. #9727

Merged
merged 19 commits into from
Mar 18, 2022
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
937a512
Add CUDF_UNREACHABLE macro.
bdice Nov 18, 2021
106b708
Add unreachable macro to type dispatcher.
bdice Nov 18, 2021
f9894d1
Add unreachable macro to AST operator dispatcher.
bdice Nov 18, 2021
8f41a74
Merge remote-tracking branch 'upstream/branch-22.02' into add-cudf-un…
bdice Nov 19, 2021
ab1cdb9
Update CUDF_UNREACHABLE docs.
bdice Nov 19, 2021
77c97ae
Throw error in host-side type dispatcher if invalid type is passed.
bdice Nov 19, 2021
2c39210
Merge remote-tracking branch 'upstream/branch-22.04' into add-cudf-un…
bdice Jan 24, 2022
2d61ea9
Use CUDF_UNREACHABLE only on device in AST operator dispatch.
bdice Jan 24, 2022
a0d9baa
Merge remote-tracking branch 'upstream/branch-22.04' into add-cudf-un…
bdice Jan 26, 2022
8c7d978
Merge remote-tracking branch 'upstream/branch-22.04' into add-cudf-un…
bdice Jan 28, 2022
52ad3a0
Use CUDF_UNREACHABLE (may be too aggressive, some cases may need to f…
bdice Jan 31, 2022
7de2aa0
Merge remote-tracking branch 'upstream/branch-22.04' into add-cudf-un…
bdice Mar 2, 2022
3bcfd60
Replace cudf_assert(false...) in device functions with CUDF_UNREACHABLE.
bdice Mar 2, 2022
c320c1f
Merge remote-tracking branch 'upstream/branch-22.04' into add-cudf-un…
bdice Mar 15, 2022
6d8d498
Merge remote-tracking branch 'upstream/branch-22.04' into add-cudf-un…
bdice Mar 17, 2022
8c2b1dc
Add comment about assert in debug mode.
bdice Mar 17, 2022
1407315
Remove unnecessary returns.
bdice Mar 17, 2022
b66c4c4
Add explicit type since auto deduction fails with CUDF_UNREACHABLE.
bdice Mar 17, 2022
3d3000c
Merge remote-tracking branch 'upstream/branch-22.04' into add-cudf-un…
bdice Mar 18, 2022
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
2 changes: 1 addition & 1 deletion cpp/benchmarks/type_dispatcher/type_dispatcher.cu
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ struct RowHandle {
template <typename T, CUDF_ENABLE_IF(not cudf::is_rep_layout_compatible<T>())>
__device__ void operator()(cudf::mutable_column_device_view source, cudf::size_type index)
{
cudf_assert(false && "Unsupported type.");
CUDF_UNREACHABLE("Unsupported type.");
}
};

Expand Down
22 changes: 9 additions & 13 deletions cpp/include/cudf/ast/detail/expression_evaluator.cuh
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2021, NVIDIA CORPORATION.
* Copyright (c) 2021-2022, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -103,7 +103,7 @@ struct value_expression_result
if constexpr (std::is_same_v<Element, T>) {
_obj = result;
} else {
cudf_assert(false && "Output type does not match container type.");
CUDF_UNREACHABLE("Output type does not match container type.");
}
}

Expand Down Expand Up @@ -178,9 +178,7 @@ struct mutable_column_expression_result
{
// Not implemented since it would require modifying the API in the parent class to accept an
// index.
cudf_assert(false && "This method is not implemented.");
// Unreachable return used to silence compiler warnings.
return {};
CUDF_UNREACHABLE("This method is not implemented.");
vuule marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -190,7 +188,7 @@ struct mutable_column_expression_result
{
// Not implemented since it would require modifying the API in the parent class to accept an
// index.
cudf_assert(false && "This method is not implemented.");
CUDF_UNREACHABLE("This method is not implemented.");
}

mutable_column_device_view& _obj; ///< The column to which the data is written.
Expand Down Expand Up @@ -334,9 +332,7 @@ struct expression_evaluator {
cudf::size_type left_row_index,
thrust::optional<cudf::size_type> right_row_index = {}) const
{
cudf_assert(false && "Unsupported type in resolve_input.");
// Unreachable return used to silence compiler warnings.
return {};
CUDF_UNREACHABLE("Unsupported type in resolve_input.");
}

/**
Expand Down Expand Up @@ -498,7 +494,7 @@ struct expression_evaluator {
op,
thread_intermediate_storage);
} else {
cudf_assert(false && "Invalid operator arity.");
CUDF_UNREACHABLE("Invalid operator arity.");
}
}
}
Expand Down Expand Up @@ -567,7 +563,7 @@ struct expression_evaluator {
IntermediateDataType<has_nulls>* thread_intermediate_storage,
possibly_null_value_t<Element, has_nulls> const& result) const
{
cudf_assert(false && "Invalid type in resolve_output.");
CUDF_UNREACHABLE("Invalid type in resolve_output.");
}
};

Expand Down Expand Up @@ -630,7 +626,7 @@ struct expression_evaluator {
detail::device_data_reference const& output,
IntermediateDataType<has_nulls>* thread_intermediate_storage) const
{
cudf_assert(false && "Invalid unary dispatch operator for the provided input.");
CUDF_UNREACHABLE("Invalid unary dispatch operator for the provided input.");
}
};

Expand Down Expand Up @@ -698,7 +694,7 @@ struct expression_evaluator {
detail::device_data_reference const& output,
IntermediateDataType<has_nulls>* thread_intermediate_storage) const
{
cudf_assert(false && "Invalid binary dispatch operator for the provided input.");
CUDF_UNREACHABLE("Invalid binary dispatch operator for the provided input.");
}
};

Expand Down
14 changes: 7 additions & 7 deletions cpp/include/cudf/ast/detail/operators.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -199,13 +199,13 @@ CUDF_HOST_DEVICE inline constexpr void ast_operator_dispatcher(ast_operator op,
case ast_operator::CAST_TO_FLOAT64:
f.template operator()<ast_operator::CAST_TO_FLOAT64>(std::forward<Ts>(args)...);
break;
default:
default: {
#ifndef __CUDA_ARCH__
CUDF_FAIL("Invalid operator.");
#else
cudf_assert(false && "Invalid operator.");
CUDF_UNREACHABLE("Invalid operator.");
#endif
break;
}
}
}

Expand Down Expand Up @@ -934,7 +934,7 @@ struct single_dispatch_binary_operator_types {
#ifndef __CUDA_ARCH__
CUDF_FAIL("Invalid binary operation.");
#else
cudf_assert(false && "Invalid binary operation.");
CUDF_UNREACHABLE("Invalid binary operation.");
#endif
}
};
Expand Down Expand Up @@ -1023,7 +1023,7 @@ struct dispatch_unary_operator_types {
#ifndef __CUDA_ARCH__
CUDF_FAIL("Invalid unary operation.");
#else
cudf_assert(false && "Invalid unary operation.");
CUDF_UNREACHABLE("Invalid unary operation.");
#endif
}
};
Expand Down Expand Up @@ -1097,7 +1097,7 @@ struct return_type_functor {
#ifndef __CUDA_ARCH__
CUDF_FAIL("Invalid binary operation. Return type cannot be determined.");
#else
cudf_assert(false && "Invalid binary operation. Return type cannot be determined.");
CUDF_UNREACHABLE("Invalid binary operation. Return type cannot be determined.");
#endif
}

Expand Down Expand Up @@ -1125,7 +1125,7 @@ struct return_type_functor {
#ifndef __CUDA_ARCH__
CUDF_FAIL("Invalid unary operation. Return type cannot be determined.");
#else
cudf_assert(false && "Invalid unary operation. Return type cannot be determined.");
CUDF_UNREACHABLE("Invalid unary operation. Return type cannot be determined.");
#endif
}
};
Expand Down
3 changes: 1 addition & 2 deletions cpp/include/cudf/column/column_device_view.cuh
Original file line number Diff line number Diff line change
Expand Up @@ -391,8 +391,7 @@ class alignas(16) column_device_view : public detail::column_device_view_base {
CUDF_ENABLE_IF(not(is_index_type<IndexType>() and std::is_unsigned_v<IndexType>))>
__device__ size_type operator()(Args&&... args)
{
cudf_assert(false and "dictionary indices must be an unsigned integral type");
return 0;
CUDF_UNREACHABLE("dictionary indices must be an unsigned integral type");
}
};

Expand Down
2 changes: 1 addition & 1 deletion cpp/include/cudf/detail/aggregation/aggregation.cuh
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ struct update_target_element {
column_device_view source,
size_type source_index) const noexcept
{
cudf_assert(false and "Invalid source type and aggregation combination.");
CUDF_UNREACHABLE("Invalid source type and aggregation combination.");
}
};

Expand Down
12 changes: 1 addition & 11 deletions cpp/include/cudf/detail/aggregation/aggregation.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1483,17 +1483,7 @@ CUDF_HOST_DEVICE inline decltype(auto) aggregation_dispatcher(aggregation::Kind
#ifndef __CUDA_ARCH__
CUDF_FAIL("Unsupported aggregation.");
#else
cudf_assert(false && "Unsupported aggregation.");

// The following code will never be reached, but the compiler generates a
// warning if there isn't a return value.

// Need to find out what the return type is in order to have a default
// return value and solve the compiler warning for lack of a default
// return
using return_type =
decltype(f.template operator()<aggregation::SUM>(std::forward<Ts>(args)...));
return return_type();
CUDF_UNREACHABLE("Unsupported aggregation.");
#endif
}
}
Expand Down
7 changes: 3 additions & 4 deletions cpp/include/cudf/detail/indexalator.cuh
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020-2021, NVIDIA CORPORATION.
* Copyright (c) 2020-2022, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -267,8 +267,7 @@ struct input_indexalator : base_indexalator<input_indexalator> {
template <typename T, std::enable_if_t<not is_index_type<T>()>* = nullptr>
__device__ size_type operator()(void const* tp)
{
cudf_assert(false and "only index types are supported");
return 0;
CUDF_UNREACHABLE("only index types are supported");
}
};
/**
Expand Down Expand Up @@ -365,7 +364,7 @@ struct output_indexalator : base_indexalator<output_indexalator> {
template <typename T, std::enable_if_t<not is_index_type<T>()>* = nullptr>
__device__ void operator()(void* tp, size_type const value)
{
cudf_assert(false and "only index types are supported");
CUDF_UNREACHABLE("only index types are supported");
}
};

Expand Down
28 changes: 27 additions & 1 deletion cpp/include/cudf/detail/utilities/assert.cuh
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019-2020, NVIDIA CORPORATION.
* Copyright (c) 2019-2022, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -35,3 +35,29 @@
#else
#define cudf_assert(e) (static_cast<void>(0))
#endif

/**
* @brief Macro indicating that a location in the code is unreachable.
*
* The CUDF_UNREACHABLE macro should only be used where CUDF_FAIL cannot be used
* due to performance or due to being used in device code. In the majority of
* host code situations, an exception should be thrown in "unreachable" code
* paths as those usually aren't tight inner loops like they are in device code.
*
* One example where this macro may be used is in conjunction with dispatchers
* to indicate that a function does not need to return a default value because
* it has already exhausted all possible cases in a `switch` statement.
*
* The assert in this macro can be used when compiling in debug mode to help
* debug functions that may reach the supposedly unreachable code.
*
* Example usage:
* ```
* CUDF_UNREACHABLE("Invalid type_id.");
* ```
*/
#define CUDF_UNREACHABLE(msg) \
do { \
assert(false && "Unreachable: " msg); \
vuule marked this conversation as resolved.
Show resolved Hide resolved
__builtin_unreachable(); \
} while (0)
17 changes: 6 additions & 11 deletions cpp/include/cudf/detail/utilities/hash_functions.cuh
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ auto __device__ inline get_element_pointer_and_size(Element const& element)
if constexpr (is_fixed_width<Element>() && !is_chrono<Element>()) {
return thrust::make_pair(reinterpret_cast<uint8_t const*>(&element), sizeof(Element));
} else {
cudf_assert(false && "Unsupported type.");
CUDF_UNREACHABLE("Unsupported type.");
}
}

Expand Down Expand Up @@ -334,16 +334,14 @@ template <>
hash_value_type __device__ inline MurmurHash3_32<cudf::list_view>::operator()(
cudf::list_view const& key) const
{
cudf_assert(false && "List column hashing is not supported");
return 0;
CUDF_UNREACHABLE("List column hashing is not supported");
}

template <>
hash_value_type __device__ inline MurmurHash3_32<cudf::struct_view>::operator()(
cudf::struct_view const& key) const
{
cudf_assert(false && "Direct hashing of struct_view is not supported");
return 0;
CUDF_UNREACHABLE("Direct hashing of struct_view is not supported");
}

template <typename Key>
Expand Down Expand Up @@ -551,16 +549,14 @@ template <>
hash_value_type __device__ inline SparkMurmurHash3_32<cudf::list_view>::operator()(
cudf::list_view const& key) const
{
cudf_assert(false && "List column hashing is not supported");
return 0;
CUDF_UNREACHABLE("List column hashing is not supported");
}

template <>
hash_value_type __device__ inline SparkMurmurHash3_32<cudf::struct_view>::operator()(
cudf::struct_view const& key) const
{
cudf_assert(false && "Direct hashing of struct_view is not supported");
return 0;
CUDF_UNREACHABLE("Direct hashing of struct_view is not supported");
}

/**
Expand All @@ -577,8 +573,7 @@ struct IdentityHash {
constexpr std::enable_if_t<!std::is_arithmetic_v<Key>, return_type> operator()(
Key const& key) const
{
cudf_assert(false && "IdentityHash does not support this data type");
return 0;
CUDF_UNREACHABLE("IdentityHash does not support this data type");
}

template <typename return_type = result_type>
Expand Down
12 changes: 4 additions & 8 deletions cpp/include/cudf/table/row_operators.cuh
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,7 @@ class element_equality_comparator {
std::enable_if_t<not cudf::is_equality_comparable<Element, Element>()>* = nullptr>
__device__ bool operator()(size_type lhs_element_index, size_type rhs_element_index)
{
cudf_assert(false && "Attempted to compare elements of uncomparable types.");
return false;
CUDF_UNREACHABLE("Attempted to compare elements of uncomparable types.");
}

private:
Expand Down Expand Up @@ -323,8 +322,7 @@ class element_relational_comparator {
std::enable_if_t<not cudf::is_relationally_comparable<Element, Element>()>* = nullptr>
__device__ weak_ordering operator()(size_type lhs_element_index, size_type rhs_element_index)
{
cudf_assert(false && "Attempted to compare elements of uncomparable types.");
return weak_ordering::LESS;
CUDF_UNREACHABLE("Attempted to compare elements of uncomparable types.");
}

private:
Expand Down Expand Up @@ -442,8 +440,7 @@ class element_hasher {
template <typename T, CUDF_ENABLE_IF(not column_device_view::has_element_accessor<T>())>
__device__ hash_value_type operator()(column_device_view col, size_type row_index) const
{
cudf_assert(false && "Unsupported type in hash.");
return {};
CUDF_UNREACHABLE("Unsupported type in hash.");
}

Nullate has_nulls;
Expand Down Expand Up @@ -472,8 +469,7 @@ class element_hasher_with_seed {
template <typename T, CUDF_ENABLE_IF(not column_device_view::has_element_accessor<T>())>
__device__ hash_value_type operator()(column_device_view col, size_type row_index) const
{
cudf_assert(false && "Unsupported type in hash.");
return {};
CUDF_UNREACHABLE("Unsupported type in hash.");
}

private:
Expand Down
13 changes: 2 additions & 11 deletions cpp/include/cudf/utilities/type_dispatcher.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -511,18 +511,9 @@ CUDF_HOST_DEVICE __forceinline__ constexpr decltype(auto) type_dispatcher(cudf::
std::forward<Ts>(args)...);
default: {
#ifndef __CUDA_ARCH__
CUDF_FAIL("Unsupported type_id.");
CUDF_FAIL("Invalid type_id.");
#else
cudf_assert(false && "Unsupported type_id.");

// The following code will never be reached, but the compiler generates a
// warning if there isn't a return value.

// Need to find out what the return type is in order to have a default
// return value and solve the compiler warning for lack of a default
// return
using return_type = decltype(f.template operator()<int8_t>(std::forward<Ts>(args)...));
return return_type();
CUDF_UNREACHABLE("Invalid type_id.");
#endif
Comment on lines 513 to 517
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful to make this into a single macro (maybe this should be CUDF_UNREACHABLE, so it covers both host and device code)? I see the pattern in a few places in the PR.

Copy link
Contributor Author

@bdice bdice Mar 17, 2022

Choose a reason for hiding this comment

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

I considered that, but I didn't want to hide the dependence on #ifndef __CUDA_ARCH__. Failure/raising an error and unreachable code mean very different things in my opinion, and I didn't want to conflate them by replacing this with an idiom that has potential for misuse. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure. It's weird because we do have the uneven handling between host and device as it is. Maybe it should be the other way around, and CUDF_FAIL can call CUDF_UNREACHABLE if in device code. As in - "we failed on the device, here's an assert if debug and don't expect a return".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tagging @jrhemstad for thoughts on this. I would defer that change to a later PR if possible.

Copy link
Contributor Author

@bdice bdice Mar 17, 2022

Choose a reason for hiding this comment

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

I think I'm still in favor of keeping these macros separate. Letting CUDF_FAIL defer to an unreachable path seems dangerous. Developers that see CUDF_FAIL should be able to reasonably expect an error, and should not use it to signify branches that can be optimized out as impossible to reach. A macro named something like CUDF_IMPOSSIBLE might be a compromise, but I think a combined macro like that would obscure the intention (in harmful ways) more than it helps with cleanliness/brevity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, obscuring the intention is the main issue I can see.

Here's what bugs me: we are using CUDF_UNREACHABLE both for truly unreachable code and failure. Ideally, CUDF_UNREACHABLE macro would call GCC's __builtin_unreachable() if in host code. But we call CUDF_FAIL instead in such cases.
Feels like code that should not be executed should use CUDF_FAIL (both host and device) and truly unreachable code should use CUDF_UNREACHABLE (both host and device). I understand that this may do more hard than good, just bringing it up for consideration.

Copy link
Contributor Author

@bdice bdice Mar 17, 2022

Choose a reason for hiding this comment

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

I believe all the cases handled in this way are actually unreachable (by enum exhaustion, in most cases). We’re just taking the opportunity to raise an error on the host because we can do that without any significant performance or compile time penalty.

}
}
Expand Down
6 changes: 2 additions & 4 deletions cpp/src/datetime/datetime_ops.cu
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,8 @@ struct RoundFunctor {
case rounding_function::CEIL: return cuda::std::chrono::ceil<DurationType>(dt);
case rounding_function::FLOOR: return cuda::std::chrono::floor<DurationType>(dt);
case rounding_function::ROUND: return cuda::std::chrono::round<DurationType>(dt);
default: cudf_assert(false && "Unsupported rounding kind.");
default: CUDF_UNREACHABLE("Unsupported rounding kind.");
}
__builtin_unreachable();
}
};

Expand Down Expand Up @@ -145,9 +144,8 @@ struct RoundingDispatcher {
case rounding_frequency::NANOSECOND:
return time_point_cast<typename Timestamp::duration>(
RoundFunctor<duration_ns>{}(round_kind, ts));
default: cudf_assert(false && "Unsupported datetime rounding resolution.");
default: CUDF_UNREACHABLE("Unsupported datetime rounding resolution.");
}
__builtin_unreachable();
}
};

Expand Down
Loading