From bd9b0a6fbfa1073c235d39c855ef5805f7e201fa Mon Sep 17 00:00:00 2001 From: Bradley Dice Date: Thu, 28 Apr 2022 17:07:42 -0500 Subject: [PATCH 1/8] Refactor binaryop/compiled/util.cpp. --- cpp/src/binaryop/compiled/util.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/cpp/src/binaryop/compiled/util.cpp b/cpp/src/binaryop/compiled/util.cpp index d8f1eb03a16..d18b3f36948 100644 --- a/cpp/src/binaryop/compiled/util.cpp +++ b/cpp/src/binaryop/compiled/util.cpp @@ -38,10 +38,10 @@ struct common_type_functor { { // If common_type exists if constexpr (cudf::has_common_type_v) { - using TypeCommon = typename std::common_type::type; + using TypeCommon = std::common_type_t; return data_type{type_to_id()}; } else if constexpr (cudf::has_common_type_v) { - using TypeCommon = typename std::common_type::type; + using TypeCommon = std::common_type_t; // Eg. d=t-t return data_type{type_to_id()}; } @@ -66,7 +66,7 @@ struct common_type_functor { */ template struct is_binary_operation_supported { - // For types where Out type is fixed. (eg. comparison types) + // For types where Out type is fixed. (e.g. comparison types) template inline constexpr bool operator()() { @@ -97,11 +97,9 @@ struct is_binary_operation_supported { return std::is_constructible_v or (is_fixed_point() and is_fixed_point()); } - } else { - if constexpr (std::is_invocable_v) { - using ReturnType = std::invoke_result_t; - return std::is_constructible_v; - } + } else if constexpr (std::is_invocable_v) { + using ReturnType = std::invoke_result_t; + return std::is_constructible_v; } } return false; From 0c10a37cccddc65d4ac5fb04ff378399b4d0186c Mon Sep 17 00:00:00 2001 From: Bradley Dice Date: Thu, 28 Apr 2022 17:41:23 -0500 Subject: [PATCH 2/8] Use two double-dispatches instead of a triple-dispatch. --- cpp/src/binaryop/compiled/util.cpp | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/cpp/src/binaryop/compiled/util.cpp b/cpp/src/binaryop/compiled/util.cpp index d18b3f36948..19c6f930458 100644 --- a/cpp/src/binaryop/compiled/util.cpp +++ b/cpp/src/binaryop/compiled/util.cpp @@ -26,6 +26,19 @@ namespace cudf::binops::compiled { namespace { + +struct common_data_type_functor { + template + std::optional operator()() + { + if constexpr (cudf::has_common_type_v) { + using TypeCommon = std::common_type_t; + return data_type{type_to_id()}; + } + return std::nullopt; + } +}; + /** * @brief Functor that returns optional common type of 2 or 3 given types. * @@ -183,7 +196,16 @@ struct is_supported_operation_functor { std::optional get_common_type(data_type out, data_type lhs, data_type rhs) { - return double_type_dispatcher(lhs, rhs, common_type_functor{}, out); + // We want std::common_type_t. We can avoid a + // triple type dispatch by using the definition of std::common_type to + // compute this with two double-dispatches. Specifically, + // std::common_type_t is the same as + // std::common_type_t, C>. + auto common_type = double_type_dispatcher(out, lhs, common_data_type_functor{}); + if (common_type.has_value()) { + common_type = double_type_dispatcher(common_type.value(), rhs, common_data_type_functor{}); + } + return common_type; } bool is_supported_operation(data_type out, data_type lhs, data_type rhs, binary_operator op) From 6f068dae5b7c78aaf4f0ee2042f42380817bb091 Mon Sep 17 00:00:00 2001 From: Bradley Dice Date: Thu, 28 Apr 2022 18:46:46 -0500 Subject: [PATCH 3/8] Fix logic for fallback case. --- cpp/src/binaryop/compiled/util.cpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/cpp/src/binaryop/compiled/util.cpp b/cpp/src/binaryop/compiled/util.cpp index 19c6f930458..f56af438d55 100644 --- a/cpp/src/binaryop/compiled/util.cpp +++ b/cpp/src/binaryop/compiled/util.cpp @@ -196,15 +196,21 @@ struct is_supported_operation_functor { std::optional get_common_type(data_type out, data_type lhs, data_type rhs) { - // We want std::common_type_t. We can avoid a - // triple type dispatch by using the definition of std::common_type to - // compute this with two double-dispatches. Specifically, - // std::common_type_t is the same as - // std::common_type_t, C>. + // Compute the common type of (out, lhs, rhs) if it exists, or the common + // type of (lhs, rhs) if it exists, else return a null optional. + // We can avoid a triple type dispatch by using the definition of + // std::common_type to compute this with double type dispatches. + // Specifically, std::common_type_t is the same as + // std::common_type_t, TypeRhs>. auto common_type = double_type_dispatcher(out, lhs, common_data_type_functor{}); if (common_type.has_value()) { common_type = double_type_dispatcher(common_type.value(), rhs, common_data_type_functor{}); } + // If no common type of (out, lhs, rhs) exists, fall back to the common type + // of (lhs, rhs). + if (!common_type.has_value()) { + common_type = double_type_dispatcher(lhs, rhs, common_data_type_functor{}); + } return common_type; } From ee2c26a92a2e4ac6b0b3066c4a5dcbfd5805e944 Mon Sep 17 00:00:00 2001 From: Bradley Dice Date: Thu, 28 Apr 2022 19:50:02 -0500 Subject: [PATCH 4/8] Remove a level of type dispatch on TypeOut. --- cpp/src/binaryop/compiled/util.cpp | 132 ++++++++++++++--------------- 1 file changed, 63 insertions(+), 69 deletions(-) diff --git a/cpp/src/binaryop/compiled/util.cpp b/cpp/src/binaryop/compiled/util.cpp index f56af438d55..c860809cfc3 100644 --- a/cpp/src/binaryop/compiled/util.cpp +++ b/cpp/src/binaryop/compiled/util.cpp @@ -39,39 +39,34 @@ struct common_data_type_functor { } }; -/** - * @brief Functor that returns optional common type of 2 or 3 given types. - * - */ -struct common_type_functor { - template - struct nested_common_type_functor { - template - std::optional operator()() - { - // If common_type exists - if constexpr (cudf::has_common_type_v) { - using TypeCommon = std::common_type_t; - return data_type{type_to_id()}; - } else if constexpr (cudf::has_common_type_v) { - using TypeCommon = std::common_type_t; - // Eg. d=t-t - return data_type{type_to_id()}; - } +struct has_mutable_element_accessor_functor { + template + bool operator()() + { + return mutable_column_device_view::has_element_accessor(); + } +}; - // A compiler bug may cause a compilation error when using empty initializer list to construct - // an std::optional object containing no `data_type` value. Therefore, we should explicitly - // return `std::nullopt` instead. - return std::nullopt; - } - }; - template - std::optional operator()(data_type out) +bool has_mutable_element_accessor(data_type t) +{ + return type_dispatcher(t, has_mutable_element_accessor_functor{}); +} + +template +struct is_constructible_functor { + template + bool operator()() { - return type_dispatcher(out, nested_common_type_functor{}); + return std::is_constructible_v; } }; +template +bool is_constructible(data_type target_type) +{ + return type_dispatcher(target_type, is_constructible_functor{}); +} + /** * @brief Functor that return true if BinaryOperator supports given input and output types. * @@ -96,23 +91,23 @@ struct is_binary_operation_supported { } } - template - inline constexpr bool operator()() + template + inline constexpr bool operator()(data_type out_type) { if constexpr (column_device_view::has_element_accessor() and - column_device_view::has_element_accessor() and - (mutable_column_device_view::has_element_accessor() or - is_fixed_point())) { - if constexpr (has_common_type_v) { - using common_t = std::common_type_t; - if constexpr (std::is_invocable_v) { - using ReturnType = std::invoke_result_t; - return std::is_constructible_v or - (is_fixed_point() and is_fixed_point()); + column_device_view::has_element_accessor()) { + if (has_mutable_element_accessor(out_type) or is_fixed_point(out_type)) { + if constexpr (has_common_type_v) { + using common_t = std::common_type_t; + if constexpr (std::is_invocable_v) { + using ReturnType = std::invoke_result_t; + return is_constructible(out_type) or + (is_fixed_point() and is_fixed_point(out_type)); + } + } else if constexpr (std::is_invocable_v) { + using ReturnType = std::invoke_result_t; + return is_constructible(out_type); } - } else if constexpr (std::is_invocable_v) { - using ReturnType = std::invoke_result_t; - return std::is_constructible_v; } } return false; @@ -122,37 +117,36 @@ struct is_binary_operation_supported { struct is_supported_operation_functor { template struct nested_support_functor { - template - inline constexpr bool call() + template + inline constexpr bool call(data_type out_type) { - return is_binary_operation_supported{} - .template operator()(); + return is_binary_operation_supported{}.template operator()( + out_type); } - template - inline constexpr bool operator()(binary_operator op) + inline constexpr bool operator()(binary_operator op, data_type out_type) { switch (op) { // clang-format off - case binary_operator::ADD: return call(); - case binary_operator::SUB: return call(); - case binary_operator::MUL: return call(); - case binary_operator::DIV: return call(); - case binary_operator::TRUE_DIV: return call(); - case binary_operator::FLOOR_DIV: return call(); - case binary_operator::MOD: return call(); - case binary_operator::PYMOD: return call(); - case binary_operator::POW: return call(); - case binary_operator::BITWISE_AND: return call(); - case binary_operator::BITWISE_OR: return call(); - case binary_operator::BITWISE_XOR: return call(); - case binary_operator::SHIFT_LEFT: return call(); - case binary_operator::SHIFT_RIGHT: return call(); - case binary_operator::SHIFT_RIGHT_UNSIGNED: return call(); - case binary_operator::LOG_BASE: return call(); - case binary_operator::ATAN2: return call(); - case binary_operator::PMOD: return call(); - case binary_operator::NULL_MAX: return call(); - case binary_operator::NULL_MIN: return call(); + case binary_operator::ADD: return call(out_type); + case binary_operator::SUB: return call(out_type); + case binary_operator::MUL: return call(out_type); + case binary_operator::DIV: return call(out_type); + case binary_operator::TRUE_DIV: return call(out_type); + case binary_operator::FLOOR_DIV: return call(out_type); + case binary_operator::MOD: return call(out_type); + case binary_operator::PYMOD: return call(out_type); + case binary_operator::POW: return call(out_type); + case binary_operator::BITWISE_AND: return call(out_type); + case binary_operator::BITWISE_OR: return call(out_type); + case binary_operator::BITWISE_XOR: return call(out_type); + case binary_operator::SHIFT_LEFT: return call(out_type); + case binary_operator::SHIFT_RIGHT: return call(out_type); + case binary_operator::SHIFT_RIGHT_UNSIGNED: return call(out_type); + case binary_operator::LOG_BASE: return call(out_type); + case binary_operator::ATAN2: return call(out_type); + case binary_operator::PMOD: return call(out_type); + case binary_operator::NULL_MAX: return call(out_type); + case binary_operator::NULL_MIN: return call(out_type); /* case binary_operator::GENERIC_BINARY: // defined in jit only. */ @@ -186,7 +180,7 @@ struct is_supported_operation_functor { return bool_op(out); case binary_operator::NULL_LOGICAL_OR: return bool_op(out); - default: return type_dispatcher(out, nested_support_functor{}, op); + default: return nested_support_functor{}(op, out); } return false; } From 1c256fb60d1128427c1a7765b7a4d62355dc7482 Mon Sep 17 00:00:00 2001 From: Bradley Dice Date: Thu, 28 Apr 2022 20:17:37 -0500 Subject: [PATCH 5/8] Add back comment. --- cpp/src/binaryop/compiled/util.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cpp/src/binaryop/compiled/util.cpp b/cpp/src/binaryop/compiled/util.cpp index c860809cfc3..2baf09ab19d 100644 --- a/cpp/src/binaryop/compiled/util.cpp +++ b/cpp/src/binaryop/compiled/util.cpp @@ -35,6 +35,11 @@ struct common_data_type_functor { using TypeCommon = std::common_type_t; return data_type{type_to_id()}; } + + // A compiler bug may cause a compilation error when using empty + // initializer list to construct an std::optional object containing no + // `data_type` value. Therefore, we explicitly return `std::nullopt` + // instead. return std::nullopt; } }; From e2e2032ffab3b10d6512cc865e83e0c36f23ab59 Mon Sep 17 00:00:00 2001 From: Bradley Dice Date: Thu, 28 Apr 2022 21:22:31 -0500 Subject: [PATCH 6/8] Rename common_data_type_functor to common_type_functor. --- cpp/src/binaryop/compiled/util.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/src/binaryop/compiled/util.cpp b/cpp/src/binaryop/compiled/util.cpp index 2baf09ab19d..c2acbe91dce 100644 --- a/cpp/src/binaryop/compiled/util.cpp +++ b/cpp/src/binaryop/compiled/util.cpp @@ -27,7 +27,7 @@ namespace cudf::binops::compiled { namespace { -struct common_data_type_functor { +struct common_type_functor { template std::optional operator()() { @@ -201,14 +201,14 @@ std::optional get_common_type(data_type out, data_type lhs, data_type // std::common_type to compute this with double type dispatches. // Specifically, std::common_type_t is the same as // std::common_type_t, TypeRhs>. - auto common_type = double_type_dispatcher(out, lhs, common_data_type_functor{}); + auto common_type = double_type_dispatcher(out, lhs, common_type_functor{}); if (common_type.has_value()) { - common_type = double_type_dispatcher(common_type.value(), rhs, common_data_type_functor{}); + common_type = double_type_dispatcher(common_type.value(), rhs, common_type_functor{}); } // If no common type of (out, lhs, rhs) exists, fall back to the common type // of (lhs, rhs). if (!common_type.has_value()) { - common_type = double_type_dispatcher(lhs, rhs, common_data_type_functor{}); + common_type = double_type_dispatcher(lhs, rhs, common_type_functor{}); } return common_type; } From be5537e68470be1a7e7dd599e318ee72820323d7 Mon Sep 17 00:00:00 2001 From: Bradley Dice Date: Fri, 29 Apr 2022 12:06:49 -0500 Subject: [PATCH 7/8] Apply const suggestions Co-authored-by: Robert Maynard --- cpp/src/binaryop/compiled/util.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/cpp/src/binaryop/compiled/util.cpp b/cpp/src/binaryop/compiled/util.cpp index c2acbe91dce..39d96266637 100644 --- a/cpp/src/binaryop/compiled/util.cpp +++ b/cpp/src/binaryop/compiled/util.cpp @@ -29,7 +29,7 @@ namespace { struct common_type_functor { template - std::optional operator()() + std::optional operator()() const { if constexpr (cudf::has_common_type_v) { using TypeCommon = std::common_type_t; @@ -46,7 +46,7 @@ struct common_type_functor { struct has_mutable_element_accessor_functor { template - bool operator()() + bool operator()() const { return mutable_column_device_view::has_element_accessor(); } @@ -60,7 +60,7 @@ bool has_mutable_element_accessor(data_type t) template struct is_constructible_functor { template - bool operator()() + bool operator()() const { return std::is_constructible_v; } @@ -81,7 +81,7 @@ template struct is_binary_operation_supported { // For types where Out type is fixed. (e.g. comparison types) template - inline constexpr bool operator()() + inline constexpr bool operator()() const { if constexpr (column_device_view::has_element_accessor() and column_device_view::has_element_accessor()) { @@ -97,7 +97,7 @@ struct is_binary_operation_supported { } template - inline constexpr bool operator()(data_type out_type) + inline constexpr bool operator()(data_type out_type) const { if constexpr (column_device_view::has_element_accessor() and column_device_view::has_element_accessor()) { @@ -123,12 +123,12 @@ struct is_supported_operation_functor { template struct nested_support_functor { template - inline constexpr bool call(data_type out_type) + inline constexpr bool call(data_type out_type) const { return is_binary_operation_supported{}.template operator()( out_type); } - inline constexpr bool operator()(binary_operator op, data_type out_type) + inline constexpr bool operator()(binary_operator op, data_type out_type) const { switch (op) { // clang-format off From 1b9656caeb317d1fb57f67d75aae523293ec5b76 Mon Sep 17 00:00:00 2001 From: Bradley Dice Date: Fri, 29 Apr 2022 12:07:34 -0500 Subject: [PATCH 8/8] Add const. --- cpp/src/binaryop/compiled/util.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/src/binaryop/compiled/util.cpp b/cpp/src/binaryop/compiled/util.cpp index 39d96266637..91fa04be6e2 100644 --- a/cpp/src/binaryop/compiled/util.cpp +++ b/cpp/src/binaryop/compiled/util.cpp @@ -162,13 +162,13 @@ struct is_supported_operation_functor { }; template - inline constexpr bool bool_op(data_type out) + inline constexpr bool bool_op(data_type out) const { return out.id() == type_id::BOOL8 and is_binary_operation_supported{}.template operator()(); } template - inline constexpr bool operator()(data_type out, binary_operator op) + inline constexpr bool operator()(data_type out, binary_operator op) const { switch (op) { // output type should be bool type.