From 7778172a1f9540a55cdc65e1470debf1758670bc Mon Sep 17 00:00:00 2001 From: Maksim Kita Date: Wed, 5 May 2021 23:08:31 +0300 Subject: [PATCH] Fixed compilable function --- src/Functions/FunctionsComparison.h | 2 +- src/Functions/greatest.cpp | 11 ++++++---- src/Functions/least.cpp | 8 ++++++-- src/Interpreters/ExpressionJIT.cpp | 26 ++++++++++++------------ src/Interpreters/JIT/CompileDAG.cpp | 5 +++++ src/Interpreters/JIT/compileFunction.cpp | 2 +- 6 files changed, 33 insertions(+), 21 deletions(-) diff --git a/src/Functions/FunctionsComparison.h b/src/Functions/FunctionsComparison.h index 9cb6b043f22..f724915b3bc 100644 --- a/src/Functions/FunctionsComparison.h +++ b/src/Functions/FunctionsComparison.h @@ -1243,7 +1243,7 @@ public: WhichDataType data_type_lhs(types[0]); WhichDataType data_type_rhs(types[1]); - auto is_big_integer = [](WhichDataType type) { return type.isUInt64() || type.isInt64() || type.isUUID(); }; + auto is_big_integer = [](WhichDataType type) { return type.isUInt64() || type.isInt64(); }; if ((is_big_integer(data_type_lhs) && data_type_rhs.isFloat()) || (is_big_integer(data_type_rhs) && data_type_lhs.isFloat()) diff --git a/src/Functions/greatest.cpp b/src/Functions/greatest.cpp index 56b5630b6bc..9fda95db01f 100644 --- a/src/Functions/greatest.cpp +++ b/src/Functions/greatest.cpp @@ -26,10 +26,13 @@ struct GreatestBaseImpl static inline llvm::Value * compile(llvm::IRBuilder<> & b, llvm::Value * left, llvm::Value * right, bool is_signed) { if (!left->getType()->isIntegerTy()) - /// XXX maxnum is basically fmax(), it may or may not match whatever apply() does - /// XXX CreateMaxNum is broken on LLVM 5.0 and 6.0 (generates minnum instead; fixed in 7) - return b.CreateBinaryIntrinsic(llvm::Intrinsic::maxnum, left, right); - return b.CreateSelect(is_signed ? b.CreateICmpSGT(left, right) : b.CreateICmpUGT(left, right), left, right); + { + /// Follows the IEEE-754 semantics for maxNum except for the handling of signaling NaNs. This matches the behavior of libc fmax. + return b.CreateMaxNum(left, right); + } + + auto * compare_value = is_signed ? b.CreateICmpSGT(left, right) : b.CreateICmpUGT(left, right); + return b.CreateSelect(compare_value, left, right); } #endif }; diff --git a/src/Functions/least.cpp b/src/Functions/least.cpp index ba87e4bd7e4..6ba7989a344 100644 --- a/src/Functions/least.cpp +++ b/src/Functions/least.cpp @@ -26,9 +26,13 @@ struct LeastBaseImpl static inline llvm::Value * compile(llvm::IRBuilder<> & b, llvm::Value * left, llvm::Value * right, bool is_signed) { if (!left->getType()->isIntegerTy()) - /// XXX minnum is basically fmin(), it may or may not match whatever apply() does + { + /// Follows the IEEE-754 semantics for minNum, except for handling of signaling NaNs. This match’s the behavior of libc fmin. return b.CreateMinNum(left, right); - return b.CreateSelect(is_signed ? b.CreateICmpSLT(left, right) : b.CreateICmpULT(left, right), left, right); + } + + auto * compare_value = is_signed ? b.CreateICmpSLT(left, right) : b.CreateICmpULT(left, right); + return b.CreateSelect(compare_value, left, right); } #endif }; diff --git a/src/Interpreters/ExpressionJIT.cpp b/src/Interpreters/ExpressionJIT.cpp index f41266e4987..784f891d151 100644 --- a/src/Interpreters/ExpressionJIT.cpp +++ b/src/Interpreters/ExpressionJIT.cpp @@ -367,26 +367,26 @@ static CompileDAG getCompilableDAG( break; } - bool should_visit_children_first = frame.next_child_to_visit < node->children.size(); + bool all_children_visited = frame.next_child_to_visit == node->children.size(); - if (should_visit_children_first) + if (all_children_visited) continue; CompileDAG::Node compile_node; compile_node.function = node->function_base; compile_node.result_type = node->result_type; - if (node->type == ActionsDAG::ActionType::FUNCTION) + if (isCompilableConstant(*node)) + { + compile_node.type = CompileDAG::CompileType::CONSTANT; + compile_node.column = node->column; + } + else if (node->type == ActionsDAG::ActionType::FUNCTION) { compile_node.type = CompileDAG::CompileType::FUNCTION; for (const auto * child : node->children) compile_node.arguments.push_back(visited_node_to_compile_dag_position[child]); } - else if (isCompilableConstant(*node)) - { - compile_node.type = CompileDAG::CompileType::CONSTANT; - compile_node.column = node->column; - } else { compile_node.type = CompileDAG::CompileType::INPUT; @@ -431,9 +431,9 @@ void ActionsDAG::compileFunctions(size_t min_count_to_compile_expression) std::stack stack; std::unordered_set visited_nodes; - /** Algorithm is go throught each node in ActionsDAG, and for each node iterate thought all children. - * and update data with their compilable status. - * After this procedure in data for each node is initialized. + /** Algorithm is to iterate over each node in ActionsDAG, and update node compilable status. + * Node is compilable if all its children are compilable and node is also compilable. + * After this procedure data for each node is initialized. */ for (auto & node : nodes) @@ -462,9 +462,9 @@ void ActionsDAG::compileFunctions(size_t min_count_to_compile_expression) break; } - bool should_visit_children_first = current_frame.next_child_to_visit < current_node->children.size(); + bool all_children_visited = current_frame.next_child_to_visit == current_node->children.size(); - if (should_visit_children_first) + if (!all_children_visited) continue; auto & current_node_data = node_to_data[current_node]; diff --git a/src/Interpreters/JIT/CompileDAG.cpp b/src/Interpreters/JIT/CompileDAG.cpp index 6755e61bb09..b97718c16e6 100644 --- a/src/Interpreters/JIT/CompileDAG.cpp +++ b/src/Interpreters/JIT/CompileDAG.cpp @@ -16,6 +16,11 @@ namespace DB { +namespace ErrorCodes +{ + extern const int LOGICAL_ERROR; +} + llvm::Value * CompileDAG::compile(llvm::IRBuilderBase & builder, Values input_nodes_values) const { assert(input_nodes_values.size() == getInputNodesCount()); diff --git a/src/Interpreters/JIT/compileFunction.cpp b/src/Interpreters/JIT/compileFunction.cpp index 9d36a8e071d..adc8b5ec39b 100644 --- a/src/Interpreters/JIT/compileFunction.cpp +++ b/src/Interpreters/JIT/compileFunction.cpp @@ -103,7 +103,7 @@ static void compileFunction(llvm::Module & module, const IFunctionBaseImpl & f) Values arguments; arguments.reserve(arg_types.size()); - for (size_t i = 0; i < arg_types.size(); ++i) // NOLINT + for (size_t i = 0; i < arg_types.size(); ++i) { auto & column = columns[i]; auto type = arg_types[i];