From f59b13a58d54ed5c447028dc4275cfed6ac38b88 Mon Sep 17 00:00:00 2001 From: Dmitrii Kovalkov Date: Sat, 16 May 2020 08:59:08 +0200 Subject: [PATCH] Fix style issues --- src/Common/ErrorCodes.cpp | 1 + src/Functions/FunctionsRandom.h | 2 +- src/Functions/PerformanceAdaptors.h | 56 +++++++++++++++++++---------- src/Functions/SIMDxorshift.cpp | 35 ++++++++++-------- src/Functions/SIMDxorshift.h | 2 +- src/Functions/TargetSpecific.cpp | 42 ++++++++++++---------- src/Functions/TargetSpecific.h | 5 +-- 7 files changed, 88 insertions(+), 55 deletions(-) diff --git a/src/Common/ErrorCodes.cpp b/src/Common/ErrorCodes.cpp index cb4c591041c..2681bd0773c 100644 --- a/src/Common/ErrorCodes.cpp +++ b/src/Common/ErrorCodes.cpp @@ -498,6 +498,7 @@ namespace ErrorCodes extern const int ALTER_OF_COLUMN_IS_FORBIDDEN = 524; extern const int INCORRECT_DISK_INDEX = 525; extern const int UNKNOWN_VOLUME_TYPE = 526; + extern const int NO_SUITABLE_FUNCTION_IMPLEMENTATION = 527; extern const int KEEPER_EXCEPTION = 999; extern const int POCO_EXCEPTION = 1000; diff --git a/src/Functions/FunctionsRandom.h b/src/Functions/FunctionsRandom.h index ae54243164f..6130ee1c2a5 100644 --- a/src/Functions/FunctionsRandom.h +++ b/src/Functions/FunctionsRandom.h @@ -113,4 +113,4 @@ public: } }; -} // namespace DB +} diff --git a/src/Functions/PerformanceAdaptors.h b/src/Functions/PerformanceAdaptors.h index f7bb8cfd6ee..f7b9c12c7cb 100644 --- a/src/Functions/PerformanceAdaptors.h +++ b/src/Functions/PerformanceAdaptors.h @@ -14,6 +14,11 @@ namespace DB { +namespace ErrorCodes +{ + extern const int NO_SUITABLE_FUNCTION_IMPLEMENTATION; +} + // TODO(dakovalkov): This is copied and pasted struct from LZ4_decompress_faster.h with little changes. struct PerformanceStatistics { @@ -92,15 +97,18 @@ struct PerformanceStatistics return choose_method; } - size_t size() const { + size_t size() const + { return data.size(); } - bool empty() const { + bool empty() const + { return size() == 0; } - void emplace_back() { + void emplace_back() + { data.emplace_back(); } @@ -113,7 +121,7 @@ struct PerformanceAdaptorOptions std::optional> implementations; }; -// Redirects IExecutableFunctionImpl::execute() and IFunction:executeImpl() to executeFunctionImpl(); +/// Redirects IExecutableFunctionImpl::execute() and IFunction:executeImpl() to executeFunctionImpl(); template class FunctionExecutor; @@ -170,28 +178,28 @@ public: : FunctionExecutor(params...) , options(std::move(options_)) { - if (isImplementationEnabled(DefaultFunction::getImplementationTag())) { + if (isImplementationEnabled(DefaultFunction::getImplementationTag())) statistics.emplace_back(); - } } - // Register alternative implementation. + /// Register alternative implementation. template void registerImplementation(TargetArch arch, Params... params) { - if (IsArchSupported(arch) && isImplementationEnabled(Function::getImplementationTag())) { + if (IsArchSupported(arch) && isImplementationEnabled(Function::getImplementationTag())) + { impls.emplace_back(std::make_shared(params...)); statistics.emplace_back(); } } bool isImplementationEnabled(const String & impl_tag) { - if (!options.implementations) { + if (!options.implementations) return true; - } - for (const auto & tag : *options.implementations) { - if (tag == impl_tag) { + + for (const auto & tag : *options.implementations) + { + if (tag == impl_tag) return true; - } } return false; } @@ -201,27 +209,37 @@ protected: size_t result, size_t input_rows_count) override { if (statistics.empty()) - throw "No implementations"; + throw Exception("All available implementations are disabled by user config", + ErrorCodes::NO_SUITABLE_FUNCTION_IMPLEMENTATION); + auto id = statistics.select(); Stopwatch watch; - if (id == impls.size()) { + + if (id == impls.size()) + { if constexpr (std::is_base_of_v) DefaultFunction::executeImpl(block, arguments, result, input_rows_count); else DefaultFunction::execute(block, arguments, result, input_rows_count); - } else { + } + else + { if constexpr (std::is_base_of_v) impls[id]->executeImpl(block, arguments, result, input_rows_count); else impls[id]->execute(block, arguments, result, input_rows_count); } watch.stop(); + // TODO(dakovalkov): Calculate something more informative. size_t rows_summary = 0; - for (auto i : arguments) { + for (auto i : arguments) + { rows_summary += block.getByPosition(i).column->size(); } - if (rows_summary >= 1000) { + + if (rows_summary >= 1000) + { statistics.data[id].update(watch.elapsedSeconds(), rows_summary); } } @@ -232,4 +250,4 @@ private: PerformanceAdaptorOptions options; }; -} // namespace DB +} diff --git a/src/Functions/SIMDxorshift.cpp b/src/Functions/SIMDxorshift.cpp index b5c8b0995ac..a8410ed957a 100644 --- a/src/Functions/SIMDxorshift.cpp +++ b/src/Functions/SIMDxorshift.cpp @@ -28,15 +28,16 @@ void RandXorshiftImpl::execute(char * output, size_t size) &mykey); constexpr int bytes_per_write = 8; - constexpr intptr_t mask = bytes_per_write - 1; - + constexpr intptr_t mask = bytes_per_write - 1; + // Process head to make output aligned. unalignedStore(output, xorshift128plus(&mykey)); output = reinterpret_cast((reinterpret_cast(output) | mask) + 1); - while (end - output > 0) { + while (end - output > 0) + { *reinterpret_cast(output) = xorshift128plus(&mykey); - output += bytes_per_write; + output += bytes_per_write; } } @@ -46,6 +47,9 @@ DECLARE_AVX2_SPECIFIC_CODE( void RandXorshiftImpl::execute(char * output, size_t size) { + if (size == 0) + return; + char * end = output + size; avx_xorshift128plus_key_t mykey; @@ -53,31 +57,34 @@ void RandXorshiftImpl::execute(char * output, size_t size) 0xa321e1523f4f88c7ULL ^ reinterpret_cast(output), &mykey); - constexpr int safe_overwrite = 15; // How many bytes we can write behind the end. + constexpr int safe_overwrite = 15; /// How many bytes we can write behind the end. constexpr int bytes_per_write = 32; - constexpr intptr_t mask = bytes_per_write - 1; + constexpr intptr_t mask = bytes_per_write - 1; - if (size + safe_overwrite < bytes_per_write) { - // size <= 16. + if (size + safe_overwrite < bytes_per_write) + { + /// size <= 16. _mm_storeu_si128(reinterpret_cast<__m128i*>(output), _mm256_extracti128_si256(avx_xorshift128plus(&mykey), 0)); return; } - // Process head to make output aligned. + /// Process head to make output aligned. _mm256_storeu_si256(reinterpret_cast<__m256i*>(output), avx_xorshift128plus(&mykey)); output = reinterpret_cast((reinterpret_cast(output) | mask) + 1); - while ((end - output) + safe_overwrite >= bytes_per_write) { + while ((end - output) + safe_overwrite >= bytes_per_write) + { _mm256_store_si256(reinterpret_cast<__m256i*>(output), avx_xorshift128plus(&mykey)); output += bytes_per_write; } - // Process tail. (end - output) <= 16. - if ((end - output) > 0) { + /// Process tail. (end - output) <= 16. + if ((end - output) > 0) + { _mm_store_si128(reinterpret_cast<__m128i*>(output), _mm256_extracti128_si256(avx_xorshift128plus(&mykey), 0)); - } + } } ) // DECLARE_AVX2_SPECIFIC_CODE @@ -93,4 +100,4 @@ void registerFunctionRandXorshift(FunctionFactory & factory) factory.registerFunction(); } -} // namespace DB +} diff --git a/src/Functions/SIMDxorshift.h b/src/Functions/SIMDxorshift.h index c8b741c06b1..c9e46cf7192 100644 --- a/src/Functions/SIMDxorshift.h +++ b/src/Functions/SIMDxorshift.h @@ -45,4 +45,4 @@ public: } }; -} // namespace DB +} diff --git a/src/Functions/TargetSpecific.cpp b/src/Functions/TargetSpecific.cpp index 19604a83ab7..891d63d8258 100644 --- a/src/Functions/TargetSpecific.cpp +++ b/src/Functions/TargetSpecific.cpp @@ -11,25 +11,30 @@ namespace DB { __attribute__ ((target("xsave"))) -uint64_t xgetbv(uint32_t ecx) { +UInt64 xgetbv(UInt32 ecx) +{ return _xgetbv(ecx); } -int GetSupportedArches() { - unsigned int eax, ebx, ecx, edx; - if (!__get_cpuid(1, &eax, &ebx, &ecx, &edx)) { +UInt32 GetSupportedArches() +{ + UInt32 eax, ebx, ecx, edx; + if (!__get_cpuid(1, &eax, &ebx, &ecx, &edx)) return 0; - } - int res = 0; + + UInt32 res = 0; if (ecx & bit_SSE4_2) - res |= static_cast(TargetArch::SSE4); - // (xgetbv(0) & 0x6) == 0x6 checks that XMM state and YMM state are enabled. - if ((ecx & bit_OSXSAVE) && (ecx & bit_AVX) && (xgetbv(0) & 0x6) == 0x6) { - res |= static_cast(TargetArch::AVX); - if (__get_cpuid(7, &eax, &ebx, &ecx, &edx) && (ebx & bit_AVX2)) { - res |= static_cast(TargetArch::AVX2); - if (ebx & bit_AVX512F) { - res |= static_cast(TargetArch::AVX512F); + res |= static_cast(TargetArch::SSE4); + /// (xgetbv(0) & 0x6) == 0x6 checks that XMM state and YMM state are enabled. + if ((ecx & bit_OSXSAVE) && (ecx & bit_AVX) && (xgetbv(0) & 0x6) == 0x6) + { + res |= static_cast(TargetArch::AVX); + if (__get_cpuid(7, &eax, &ebx, &ecx, &edx) && (ebx & bit_AVX2)) + { + res |= static_cast(TargetArch::AVX2); + if (ebx & bit_AVX512F) + { + res |= static_cast(TargetArch::AVX512F); } } } @@ -38,13 +43,14 @@ int GetSupportedArches() { bool IsArchSupported(TargetArch arch) { - static int arches = GetSupportedArches(); - return arch == TargetArch::Default || (arches & static_cast(arch)); + static UInt32 arches = GetSupportedArches(); + return arch == TargetArch::Default || (arches & static_cast(arch)); } String ToString(TargetArch arch) { - switch (arch) { + switch (arch) + { case TargetArch::Default: return "default"; case TargetArch::SSE4: return "sse4"; case TargetArch::AVX: return "avx"; @@ -55,4 +61,4 @@ String ToString(TargetArch arch) __builtin_unreachable(); } -} // namespace DB +} diff --git a/src/Functions/TargetSpecific.h b/src/Functions/TargetSpecific.h index 888d88d1d77..7af792ae3c7 100644 --- a/src/Functions/TargetSpecific.h +++ b/src/Functions/TargetSpecific.h @@ -64,7 +64,8 @@ namespace DB { -enum class TargetArch : int { +enum class TargetArch : UInt32 +{ Default = 0, // Without any additional compiler options. SSE4 = (1 << 0), AVX = (1 << 1), @@ -172,4 +173,4 @@ DECLARE_AVX512F_SPECIFIC_CODE( constexpr auto BuildArch = TargetArch::AVX512F; ) // DECLARE_AVX512F_SPECIFIC_CODE -} // namespace DB +}