Vector comparisons #2119
Vector comparisons #2119
Conversation
willdealtry
force-pushed
the
vector_min_max
branch
from
df5ab5d to
5e46644
Compare
|
Label error. Requires exactly 1 of: patch, minor, major. Found: |
willdealtry
force-pushed
the
vector_min_max
branch
from
f4a95ea to
ecf54d5
Compare
| list(APPEND arcticdb_core_libraries ${KERBEROS_LIBRARY}) | ||
| list(APPEND arcticdb_core_includes ${KERBEROS_INCLUDE_DIR}) | ||
| #list(APPEND arcticdb_core_libraries ${KERBEROS_LIBRARY}) | ||
| #list(APPEND arcticdb_core_includes ${KERBEROS_INCLUDE_DIR}) |
| @@ -1097,7 +1105,7 @@ if(${TEST}) | |||
| util/test/rapidcheck_string_pool.cpp | |||
| util/test/rapidcheck_main.cpp | |||
| util/test/rapidcheck_lru_cache.cpp | |||
| version/test/rapidcheck_version_map.cpp) | |||
| version/test/rapidcheck_version_map.cpp util/test/test_min_max_integer.cpp) | |||
There was a problem hiding this comment.
Not needed here, although this code is a good candidate for rapidcheck testing
| static_assert(HeaderSize == Align); | ||
| uint8_t data_[MinSize]; | ||
| static const size_t DataAlignment = 64; | ||
| static const size_t PadSize = (DataAlignment - (HeaderDataSize % DataAlignment)) % DataAlignment; |
There was a problem hiding this comment.
mod operator is a no-op, (DataAlignment - (HeaderDataSize % DataAlignment)) < DataAlignment, so (DataAlignment - (HeaderDataSize % DataAlignment)) % DataAlignment == (DataAlignment - (HeaderDataSize % DataAlignment))
| @@ -68,7 +68,7 @@ std::vector<ChunkedBufferImpl<BlockSize>> split(const ChunkedBufferImpl<BlockSiz | |||
| } | |||
|
|
|||
| template std::vector<ChunkedBufferImpl<64>> split(const ChunkedBufferImpl<64>& input, size_t nbytes); | |||
| template std::vector<ChunkedBufferImpl<3968>> split(const ChunkedBufferImpl<3968>& input, size_t nbytes); | |||
| template std::vector<ChunkedBufferImpl<4032ul>> split(const ChunkedBufferImpl<4032ul>& input, size_t nbytes); | |||
There was a problem hiding this comment.
Was this just too small in error before?
There was a problem hiding this comment.
If there's any data that has been written into storage in 3968 byte blocks, will they still be readable without further modifications?
| @@ -19,8 +19,7 @@ class ColumnDataRandomAccessorTest : public testing::Test { | |||
| input_data.resize(n); | |||
| std::iota(input_data.begin(), input_data.end(), 42); | |||
| } | |||
| // 3968 bytes == 496 int64s per block, so 3 blocks here | |||
| size_t n{1000}; | |||
| size_t n{1100}; | |||
There was a problem hiding this comment.
Comment is useful, it's now 504 int64s per block
| constexpr size_t lane_count = sizeof(VectorType) / sizeof(T); | ||
|
|
||
| T init_min = std::numeric_limits<T>::max(); | ||
| T init_max = std::numeric_limits<T>::min(); |
There was a problem hiding this comment.
It smells a bit that there are max related variables when only min is being computed and vice versa, but I can't see a way to refactor that removes them without a lot of code duplication
|
|
||
| VectorType vector_min, vector_max; | ||
| if constexpr (ComputeMin) { | ||
| T* lanes = reinterpret_cast<T*>(&vector_min); |
There was a problem hiding this comment.
I don't think this is needed, vector_min[i] should do the obvious thing
https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html
|
|
||
| #ifdef HAS_VECTOR_EXTENSIONS | ||
|
|
||
| class MinMaxStressTest : public ::testing::Test { |
There was a problem hiding this comment.
Could move to a benchmark
There was a problem hiding this comment.
There's a lot of duplication in here, could we parametrize across supported types, and then move the random tests to rapidcheck?
| return FloatExtremumFinder<T, FloatMaxComparator<T>>::find(data, n); | ||
| } | ||
|
|
||
| #else |
There was a problem hiding this comment.
Same comments below the #else as for min_max_integer.hpp
Implement some portable vectorized comparisons