From 81d444e4e4a146fdc503ff335ddb5cf0238cce4f Mon Sep 17 00:00:00 2001 From: costan Date: Sun, 6 Jan 2019 11:48:31 -0800 Subject: [PATCH] Remove direct use of _builtin_clz. A previous CL introduced _builtin_clz in zippy.cc. This is a GCC / Clang intrinsic, and is not supported in Visual Studio. The rest of the project uses bit manipulation intrinsics via the functions in Bits::, which are stubbed out for the open source build in zippy-stubs-internal.h. This CL extracts Bits::Log2FloorNonZero() out of Bits::Log2Floor() in the stubbed version of Bits, adds assertions to the Bits::*NonZero() functions in the stubs, and converts _builtin_clz to a Bits::Log2FloorNonZero() call. The latter part is not obvious. A mathematical proof of correctness is outlined in added comments. An empirical proof is available at https://godbolt.org/z/mPKWmh -- CalculateTableSizeOld(), which is the current code, compiles to the same assembly on Clang as CalculateTableSizeNew1(), which is the bigger jump in the proof. CalculateTableSizeNew2() is a fairly obvious transformation from CalculateTableSizeNew1(), and results in slightly better assembly on all supported compilers. Two benchmark runs with the same arguments as the original CL only showed differences in completely disjoint tests, suggesting that the differences are pure noise. --- snappy-stubs-internal.h | 55 ++++++++++++++++++++++++++++++++--------- snappy.cc | 4 ++- 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/snappy-stubs-internal.h b/snappy-stubs-internal.h index f834bdb..4ea26a4 100644 --- a/snappy-stubs-internal.h +++ b/snappy-stubs-internal.h @@ -365,6 +365,9 @@ class LittleEndian { // Some bit-manipulation functions. class Bits { public: + // Return floor(log2(n)) for positive integer n. + static int Log2FloorNonZero(uint32 n); + // Return floor(log2(n)) for positive integer n. Returns -1 iff n == 0. static int Log2Floor(uint32 n); @@ -385,50 +388,72 @@ class Bits { #ifdef HAVE_BUILTIN_CTZ +inline int Bits::Log2FloorNonZero(uint32 n) { + assert(n != 0); + // (31 ^ x) is equivalent to (31 - x) for x in [0, 31]. An easy proof + // represents subtraction in base 2 and observes that there's no carry. + // + // GCC and Clang reprenset __builtin_clz on x86 as 31 ^ _bit_scan_reverse(x). + // Using "31 ^" here instead of "31 -" allows the optimizer to strip the + // function body down to _bit_scan_reverse(x). + return 31 ^ __builtin_clz(arg); +} + inline int Bits::Log2Floor(uint32 n) { - return n == 0 ? -1 : 31 ^ __builtin_clz(n); + return (n == 0) ? -1 : Bits::Log2FloorNonZero(arg); } inline int Bits::FindLSBSetNonZero(uint32 n) { + assert(n != 0); return __builtin_ctz(n); } #if defined(ARCH_K8) || defined(ARCH_PPC) || defined(ARCH_ARM) inline int Bits::FindLSBSetNonZero64(uint64 n) { + assert(n != 0); return __builtin_ctzll(n); } #endif // defined(ARCH_K8) || defined(ARCH_PPC) || defined(ARCH_ARM) #elif defined(_MSC_VER) +inline int Bits::Log2FloorNonZero(uint32 n) { + assert(n != 0); + unsigned long where; + _BitScanReverse(&where, n); + return static_cast(where); +} + inline int Bits::Log2Floor(uint32 n) { unsigned long where; - if (_BitScanReverse(&where, n)) { - return where; - } else { - return -1; - } + if (_BitScanReverse(&where, n)) + return static_cast(where); + return -1; } inline int Bits::FindLSBSetNonZero(uint32 n) { + assert(n != 0); unsigned long where; - if (_BitScanForward(&where, n)) return static_cast(where); + if (_BitScanForward(&where, n)) + return static_cast(where); return 32; } #if defined(ARCH_K8) || defined(ARCH_PPC) || defined(ARCH_ARM) inline int Bits::FindLSBSetNonZero64(uint64 n) { + assert(n != 0); unsigned long where; - if (_BitScanForward64(&where, n)) return static_cast(where); + if (_BitScanForward64(&where, n)) + return static_cast(where); return 64; } #endif // defined(ARCH_K8) || defined(ARCH_PPC) || defined(ARCH_ARM) #else // Portable versions. -inline int Bits::Log2Floor(uint32 n) { - if (n == 0) - return -1; +inline int Bits::Log2FloorNonZero(uint32 n) { + assert(n != 0); + int log = 0; uint32 value = n; for (int i = 4; i >= 0; --i) { @@ -443,7 +468,13 @@ inline int Bits::Log2Floor(uint32 n) { return log; } +inline int Bits::Log2Floor(uint32 n) { + return (n == 0) ? -1 : Bits::Log2FloorNonZero(arg); +} + inline int Bits::FindLSBSetNonZero(uint32 n) { + assert(n != 0); + int rc = 31; for (int i = 4, shift = 1 << 4; i >= 0; --i) { const uint32 x = n << shift; @@ -459,6 +490,8 @@ inline int Bits::FindLSBSetNonZero(uint32 n) { #if defined(ARCH_K8) || defined(ARCH_PPC) || defined(ARCH_ARM) // FindLSBSetNonZero64() is defined in terms of FindLSBSetNonZero(). inline int Bits::FindLSBSetNonZero64(uint64 n) { + assert(n != 0); + const uint32 bottombits = static_cast(n); if (bottombits == 0) { // Bottom bits are zero, so scan in top bits diff --git a/snappy.cc b/snappy.cc index e993c13..fad78e9 100644 --- a/snappy.cc +++ b/snappy.cc @@ -429,7 +429,9 @@ uint32 CalculateTableSize(uint32 input_size) { if (input_size < 256) { return 256; } - return 1u << (32 - __builtin_clz(input_size - 1)); + // This is equivalent to Log2Ceiling(input_size), assuming input_size > 1. + // 2 << Log2Floor(x - 1) is equivalent to 1 << (1 + Log2Floor(x - 1)). + return 2u << Bits::Log2Floor(input_size - 1); } } // namespace