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 tests build without relaxed constexpr #17691

Merged
merged 4 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
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
12 changes: 9 additions & 3 deletions cpp/include/cudf/detail/utilities/integer_utils.hpp
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright 2019 BlazingDB, Inc.
* Copyright 2019 Eyal Rozenberg <[email protected]>
* Copyright (c) 2020-2024, NVIDIA CORPORATION.
* Copyright (c) 2020-2025, 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 All @@ -23,6 +23,8 @@
*/

#include <cudf/fixed_point/temporary.hpp>
#include <cudf/types.hpp>
#include <cudf/utilities/error.hpp>

#include <cmath>
#include <cstdlib>
Expand All @@ -44,13 +46,17 @@ namespace util {
* `modulus` is positive. The safety is in regard to rollover.
*/
template <typename S>
constexpr S round_up_safe(S number_to_round, S modulus)
CUDF_HOST_DEVICE constexpr S round_up_safe(S number_to_round, S modulus)
{
auto remainder = number_to_round % modulus;
if (remainder == 0) { return number_to_round; }
auto rounded_up = number_to_round - remainder + modulus;
if (rounded_up < number_to_round) {
throw std::invalid_argument("Attempt to round up beyond the type's maximum value");
#ifndef __CUDA_ARCH__
CUDF_FAIL("Attempt to round up beyond the type's maximum value", cudf::data_type_error);
#else
CUDF_UNREACHABLE("Attempt to round up beyond the type's maximum value");
#endif
}
return rounded_up;
}
Expand Down
40 changes: 24 additions & 16 deletions cpp/include/cudf/utilities/span.hpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020-2024, NVIDIA CORPORATION.
* Copyright (c) 2020-2025, 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 @@ -197,11 +197,16 @@ struct host_span : public cudf::detail::span_base<T, Extent, host_span<T, Extent

constexpr host_span() noexcept : base() {} // required to compile on centos

/// Constructor from pointer and size
/// @param data Pointer to the first element in the span
/// @param size The number of elements in the span
/// @param is_device_accessible Whether the data is device accessible (e.g. pinned memory)
constexpr host_span(T* data, std::size_t size, bool is_device_accessible)
/**
* @brief Constructor from pointer and size
*
* @note This needs to be host-device , as it's used by a host-device function in base_2dspan
*
* @param data Pointer to the first element in the span
* @param size The number of elements in the span
* @param is_device_accessible Whether the data is device accessible (e.g. pinned memory)
*/
CUDF_HOST_DEVICE constexpr host_span(T* data, std::size_t size, bool is_device_accessible)
Copy link
Member Author

@PointKernel PointKernel Jan 7, 2025

Choose a reason for hiding this comment

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

This is not ideal but necessary because base_2dspan serves as a convenient base class, while host_2dspan and device_2dspan are essentially type aliases of the base. As a result, all member functions in the base must be marked as __host__ __device__. Consequently, the host span constructor invoked by these base member functions must also be __host__ __device__.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this tradeoff is worthwhile, but could we add a comment indicating that this is the rationale? When I first started trying to remove --expt-relaxed-constexpr I considered going the opposite route and pushing a lot of logic down from base_2dspan rather than dealing with this.

When we move to C++20 we'll be able to use std::span (and presumably cuda::std::span) so hopefully this becomes a moot point then.

Copy link
Member Author

@PointKernel PointKernel Jan 9, 2025

Choose a reason for hiding this comment

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

could we add a comment indicating that this is the rationale?

Good point. Will do Done

Actually, cuda::std::span is backported to C++17, so it is already available for use. However, I’m not sure if it is mature enough to replace our custom device_span and host_span.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think we may as well wait for C++20 so we can use host_span properly.

: base(data, size), _is_device_accessible{is_device_accessible}
{
}
Expand Down Expand Up @@ -311,8 +316,8 @@ struct host_span : public cudf::detail::span_base<T, Extent, host_span<T, Extent
* @param count The number of elements in the subspan
* @return A subspan of the sequence, of requested count and offset
*/
[[nodiscard]] constexpr host_span subspan(typename base::size_type offset,
typename base::size_type count) const noexcept
[[nodiscard]] CUDF_HOST_DEVICE constexpr host_span subspan(
typename base::size_type offset, typename base::size_type count) const noexcept
{
return host_span{this->data() + offset, count, _is_device_accessible};
}
Expand Down Expand Up @@ -434,8 +439,8 @@ struct device_span : public cudf::detail::span_base<T, Extent, device_span<T, Ex
* @param count The number of elements in the subspan
* @return A subspan of the sequence, of requested count and offset
*/
[[nodiscard]] constexpr device_span subspan(typename base::size_type offset,
typename base::size_type count) const noexcept
[[nodiscard]] CUDF_HOST_DEVICE constexpr device_span subspan(
typename base::size_type offset, typename base::size_type count) const noexcept
{
return device_span{this->data() + offset, count};
}
Expand Down Expand Up @@ -475,28 +480,28 @@ class base_2dspan {
*
* @return A pointer to the first element of the span
*/
[[nodiscard]] constexpr auto data() const noexcept { return _flat.data(); }
[[nodiscard]] CUDF_HOST_DEVICE constexpr auto data() const noexcept { return _flat.data(); }

/**
* @brief Returns the size in the span as pair.
*
* @return pair representing rows and columns size of the span
*/
[[nodiscard]] constexpr auto size() const noexcept { return _size; }
[[nodiscard]] CUDF_HOST_DEVICE constexpr auto size() const noexcept { return _size; }

/**
* @brief Returns the number of elements in the span.
*
* @return Number of elements in the span
*/
[[nodiscard]] constexpr auto count() const noexcept { return _flat.size(); }
[[nodiscard]] CUDF_HOST_DEVICE constexpr auto count() const noexcept { return _flat.size(); }

/**
* @brief Checks if the span is empty.
*
* @return True if the span is empty, false otherwise
*/
[[nodiscard]] constexpr bool is_empty() const noexcept { return count() == 0; }
[[nodiscard]] CUDF_HOST_DEVICE constexpr bool is_empty() const noexcept { return count() == 0; }

/**
* @brief Returns a reference to the row-th element of the sequence.
Expand All @@ -507,7 +512,7 @@ class base_2dspan {
* @param row the index of the element to access
* @return A reference to the row-th element of the sequence, i.e., `data()[row]`
*/
constexpr RowType<T, dynamic_extent> operator[](size_t row) const
CUDF_HOST_DEVICE constexpr RowType<T, dynamic_extent> operator[](size_t row) const
{
return _flat.subspan(row * _size.second, _size.second);
}
Expand All @@ -517,7 +522,10 @@ class base_2dspan {
*
* @return A flattened span of the 2D span
*/
[[nodiscard]] constexpr RowType<T, dynamic_extent> flat_view() const { return _flat; }
[[nodiscard]] CUDF_HOST_DEVICE constexpr RowType<T, dynamic_extent> flat_view() const
{
return _flat;
}

/**
* @brief Construct a 2D span from another 2D span of convertible type
Expand Down
49 changes: 26 additions & 23 deletions cpp/src/io/utilities/parsing_utils.cuh
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020-2024, NVIDIA CORPORATION.
* Copyright (c) 2020-2025, 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 @@ -171,7 +171,10 @@ constexpr uint8_t decode_digit(char c, bool* valid_flag)
}

// Converts character to lowercase.
constexpr char to_lower(char const c) { return c >= 'A' && c <= 'Z' ? c + ('a' - 'A') : c; }
CUDF_HOST_DEVICE constexpr char to_lower(char const c)
{
return c >= 'A' && c <= 'Z' ? c + ('a' - 'A') : c;
}

/**
* @brief Checks if string is infinity, case insensitive with/without sign
Expand Down Expand Up @@ -515,13 +518,13 @@ struct ConvertFunctor {
template <typename T,
CUDF_ENABLE_IF(std::is_integral_v<T> and !std::is_same_v<T, bool> and
!cudf::is_fixed_point<T>())>
__host__ __device__ __forceinline__ bool operator()(char const* begin,
char const* end,
void* out_buffer,
size_t row,
data_type const output_type,
parse_options_view const& opts,
bool as_hex = false)
__device__ __forceinline__ bool operator()(char const* begin,
char const* end,
void* out_buffer,
size_t row,
data_type const output_type,
parse_options_view const& opts,
bool as_hex = false)
{
auto const value = [as_hex, &opts, begin, end]() -> cuda::std::optional<T> {
// Check for user-specified true/false values
Expand Down Expand Up @@ -564,13 +567,13 @@ struct ConvertFunctor {
* @brief Dispatch for boolean type types.
*/
template <typename T, CUDF_ENABLE_IF(std::is_same_v<T, bool>)>
__host__ __device__ __forceinline__ bool operator()(char const* begin,
char const* end,
void* out_buffer,
size_t row,
data_type const output_type,
parse_options_view const& opts,
bool as_hex)
__device__ __forceinline__ bool operator()(char const* begin,
char const* end,
void* out_buffer,
size_t row,
data_type const output_type,
parse_options_view const& opts,
bool as_hex)
{
auto const value = [&opts, begin, end]() -> cuda::std::optional<T> {
// Check for user-specified true/false values
Expand All @@ -593,13 +596,13 @@ struct ConvertFunctor {
* is not valid. In such case, the validity mask is set to zero too.
*/
template <typename T, CUDF_ENABLE_IF(std::is_floating_point_v<T>)>
__host__ __device__ __forceinline__ bool operator()(char const* begin,
char const* end,
void* out_buffer,
size_t row,
data_type const output_type,
parse_options_view const& opts,
bool as_hex)
__device__ __forceinline__ bool operator()(char const* begin,
char const* end,
void* out_buffer,
size_t row,
data_type const output_type,
parse_options_view const& opts,
bool as_hex)
{
auto const value = [&opts, begin, end]() -> cuda::std::optional<T> {
// Check for user-specified true/false values
Expand Down
8 changes: 3 additions & 5 deletions cpp/src/io/utilities/trie.cuh
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2018-2024, NVIDIA CORPORATION.
* Copyright (c) 2018-2025, 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 @@ -74,16 +74,14 @@ CUDF_EXPORT trie create_serialized_trie(std::vector<std::string> const& keys,
/*
* @brief Searches for a string in a serialized trie.
*
* Can be executed on host or device, as long as the data is available
*
* @param trie Pointer to the array of nodes that make up the trie
* @param key Pointer to the start of the string to find
* @param key_len Length of the string to find
*
* @return Boolean value; true if string is found, false otherwise
*/
CUDF_HOST_DEVICE inline bool serialized_trie_contains(device_span<serial_trie_node const> trie,
device_span<char const> key)
__device__ inline bool serialized_trie_contains(device_span<serial_trie_node const> trie,
device_span<char const> key)
{
if (trie.empty()) { return false; }
if (key.empty()) { return trie.front().is_leaf; }
Expand Down
4 changes: 2 additions & 2 deletions cpp/tests/transform/segmented_row_bit_count_test.cu
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2024, NVIDIA CORPORATION.
* Copyright (c) 2024-2025, 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 @@ -74,7 +74,7 @@ compute_segmented_row_bit_count(cudf::table_view const& input, cudf::size_type s
// Since the number of rows may not divisible by segment_length,
// the last segment may be shorter than the others.
auto const size_begin = d_sizes + segment_idx * segment_length;
auto const size_end = std::min(size_begin + segment_length, d_sizes + num_rows);
auto const size_end = cuda::std::min(size_begin + segment_length, d_sizes + num_rows);
return thrust::reduce(thrust::seq, size_begin, size_end);
}));

Expand Down
18 changes: 11 additions & 7 deletions cpp/tests/utilities/column_utilities.cu
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019-2024, NVIDIA CORPORATION.
* Copyright (c) 2019-2025, 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 @@ -37,6 +37,8 @@
#include <rmm/exec_policy.hpp>

#include <cuda/functional>
#include <cuda/std/cmath>
#include <cuda/std/limits>
#include <thrust/copy.h>
#include <thrust/distance.h>
#include <thrust/equal.h>
Expand Down Expand Up @@ -412,14 +414,16 @@ class corresponding_rows_not_equivalent {
T const y = rhs.element<T>(rhs_index);

// Must handle inf and nan separately
if (std::isinf(x) || std::isinf(y)) {
if (cuda::std::isinf(x) || cuda::std::isinf(y)) {
return x != y; // comparison of (inf==inf) returns true
} else if (std::isnan(x) || std::isnan(y)) {
return std::isnan(x) != std::isnan(y); // comparison of (nan==nan) returns false
} else if (cuda::std::isnan(x) || cuda::std::isnan(y)) {
return cuda::std::isnan(x) !=
cuda::std::isnan(y); // comparison of (nan==nan) returns false
} else {
T const abs_x_minus_y = std::abs(x - y);
return abs_x_minus_y >= std::numeric_limits<T>::min() &&
abs_x_minus_y > std::numeric_limits<T>::epsilon() * std::abs(x + y) * fp_ulps;
T const abs_x_minus_y = cuda::std::abs(x - y);
return abs_x_minus_y >= cuda::std::numeric_limits<T>::min() &&
abs_x_minus_y >
cuda::std::numeric_limits<T>::epsilon() * cuda::std::abs(x + y) * fp_ulps;
}
} else {
// if either is null, then the inequality was checked already
Expand Down
Loading