diff --git a/tree/ntuple/src/RColumnElement.cxx b/tree/ntuple/src/RColumnElement.cxx index fbba02d5ed681..68b6b06a703e7 100644 --- a/tree/ntuple/src/RColumnElement.cxx +++ b/tree/ntuple/src/RColumnElement.cxx @@ -213,7 +213,12 @@ void ROOT::Internal::BitPacking::PackBits(void *dst, const void *src, std::size_ std::size_t dstIdx = 0; for (std::size_t i = 0; i < count; ++i) { Word_t packedWord = 0; +#if R__LITTLE_ENDIAN == 0 + memcpy(reinterpret_cast(&packedWord) + (sizeof(Word_t) - sizeofSrc), srcArray + i * sizeofSrc, + sizeofSrc); +#else memcpy(&packedWord, srcArray + i * sizeofSrc, sizeofSrc); +#endif // truncate the LSB of the item packedWord >>= sizeofSrc * 8 - nDstBits; @@ -232,6 +237,7 @@ void ROOT::Internal::BitPacking::PackBits(void *dst, const void *src, std::size_ accum |= (packedWordLsb << bitsUsed); } + ByteSwapIfNecessary(accum); memcpy(&dstArray[dstIdx++], &accum, sizeof(accum)); accum = 0; bitsUsed = 0; @@ -249,8 +255,10 @@ void ROOT::Internal::BitPacking::PackBits(void *dst, const void *src, std::size_ } } - if (bitsUsed) + if (bitsUsed) { + ByteSwapIfNecessary(accum); memcpy(&dstArray[dstIdx++], &accum, (bitsUsed + 7) / 8); + } [[maybe_unused]] auto expDstCount = (count * nDstBits + kBitsPerWord - 1) / kBitsPerWord; assert(dstIdx == expDstCount); @@ -278,6 +286,7 @@ void ROOT::Internal::BitPacking::UnpackBits(void *dst, const void *src, std::siz Word_t packedBytes = 0; std::size_t bytesLoaded = std::min(remBytesToLoad, sizeof(Word_t)); memcpy(&packedBytes, &srcArray[i], bytesLoaded); + ByteSwapIfNecessary(packedBytes); assert(remBytesToLoad >= bytesLoaded); remBytesToLoad -= bytesLoaded; @@ -289,7 +298,12 @@ void ROOT::Internal::BitPacking::UnpackBits(void *dst, const void *src, std::siz std::uint32_t msb = packedBytes << (8 * sizeofDst - nMsb); Word_t packedWord = msb | prevWordLsb; prevWordLsb = 0; +#if R__LITTLE_ENDIAN == 0 + memcpy(dstArray + dstIdx * sizeofDst, + reinterpret_cast(&packedWord) + sizeof(Word_t) - sizeofDst, sizeofDst); +#else memcpy(dstArray + dstIdx * sizeofDst, &packedWord, sizeofDst); +#endif ++dstIdx; offInWord = nMsb; } @@ -311,7 +325,12 @@ void ROOT::Internal::BitPacking::UnpackBits(void *dst, const void *src, std::siz assert(nSrcBits + offInWord <= kBitsPerWord); packedWord >>= offInWord; packedWord <<= 8 * sizeofDst - nSrcBits; +#if R__LITTLE_ENDIAN == 0 + memcpy(dstArray + dstIdx * sizeofDst, + reinterpret_cast(&packedWord) + sizeof(Word_t) - sizeofDst, sizeofDst); +#else memcpy(dstArray + dstIdx * sizeofDst, &packedWord, sizeofDst); +#endif ++dstIdx; offInWord += nSrcBits; } diff --git a/tree/ntuple/src/RColumnElement.hxx b/tree/ntuple/src/RColumnElement.hxx index 99420e2cd0b17..a1cba707103c6 100644 --- a/tree/ntuple/src/RColumnElement.hxx +++ b/tree/ntuple/src/RColumnElement.hxx @@ -925,16 +925,7 @@ public: R__ASSERT(GetPackedSize(count) == MinBufSize(count, fBitsOnStorage)); -#if R__LITTLE_ENDIAN == 0 - // TODO(gparolini): to avoid this extra allocation we might want to perform byte swapping - // directly in the Pack/UnpackBits functions. - auto bswapped = MakeUninitArray(count); - CopyBswap(bswapped.get(), src, count); - const auto *srcLe = bswapped.get(); -#else - const auto *srcLe = reinterpret_cast(src); -#endif - PackBits(dst, srcLe, count, sizeof(float), fBitsOnStorage); + PackBits(dst, src, count, sizeof(float), fBitsOnStorage); } void Unpack(void *dst, const void *src, std::size_t count) const final @@ -944,9 +935,6 @@ public: R__ASSERT(GetPackedSize(count) == MinBufSize(count, fBitsOnStorage)); UnpackBits(dst, src, count, sizeof(float), fBitsOnStorage); -#if R__LITTLE_ENDIAN == 0 - InPlaceBswap(dst, count); -#endif } }; @@ -966,16 +954,7 @@ public: for (std::size_t i = 0; i < count; ++i) srcFloat[i] = static_cast(srcDouble[i]); -#if R__LITTLE_ENDIAN == 0 - // TODO(gparolini): to avoid this extra allocation we might want to perform byte swapping - // directly in the Pack/UnpackBits functions. - auto bswapped = MakeUninitArray(count); - CopyBswap(bswapped.get(), srcFloat.get(), count); - const float *srcLe = bswapped.get(); -#else - const float *srcLe = reinterpret_cast(srcFloat.get()); -#endif - PackBits(dst, srcLe, count, sizeof(float), fBitsOnStorage); + PackBits(dst, reinterpret_cast(srcFloat.get()), count, sizeof(float), fBitsOnStorage); } void Unpack(void *dst, const void *src, std::size_t count) const final @@ -987,9 +966,6 @@ public: // TODO(gparolini): avoid this allocation auto dstFloat = MakeUninitArray(count); UnpackBits(dstFloat.get(), src, count, sizeof(float), fBitsOnStorage); -#if R__LITTLE_ENDIAN == 0 - InPlaceBswap(dstFloat.get(), count); -#endif double *dstDouble = reinterpret_cast(dst); for (std::size_t i = 0; i < count; ++i) @@ -1033,7 +1009,6 @@ int QuantizeReals(Quantized_t *dst, const T *src, std::size_t count, double min, const double e = 0.5 + (elem - min) * scale; Quantized_t q = static_cast(e); - ByteSwapIfNecessary(q); // double-check we actually used at most `nQuantBits` assert(outOfRange || ROOT::Internal::LeadingZeroes(q) >= unusedBits); @@ -1068,7 +1043,6 @@ int UnquantizeReals(T *dst, const Quantized_t *src, std::size_t count, double mi // Undo the LSB-preserving shift performed by QuantizeReals assert(ROOT::Internal::TrailingZeroes(elem) >= unusedBits); elem >>= unusedBits; - ByteSwapIfNecessary(elem); const double fq = static_cast(elem); double e = fq * scale + min; diff --git a/tree/ntuple/test/ntuple_endian.cxx b/tree/ntuple/test/ntuple_endian.cxx index c15498cce076b..4144f2eb02ea3 100644 --- a/tree/ntuple/test/ntuple_endian.cxx +++ b/tree/ntuple/test/ntuple_endian.cxx @@ -224,27 +224,3 @@ TEST(RColumnElementEndian, DeltaSplit) 32)); #endif } - -TEST(RColumnElementEndian, Real32Trunc) -{ -#ifndef R__BYTESWAP - GTEST_SKIP() << "Skipping test on big endian node"; -#else - RColumnElement element; - element.SetBitsOnStorage(12); - - RPageSinkMock sink1(element); - unsigned char buf1[] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, - 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f}; - RPage page1(buf1, nullptr, sizeof(float), 4); - page1.GrowUnchecked(4); - sink1.CommitPage(RPageStorage::ColumnHandle_t{}, page1); - - EXPECT_EQ(0, memcmp(sink1.GetPages()[0].GetBuffer(), "\x00\x00\x04\x80\x00\x0c", 6)); - - RPageSourceMock source1(sink1.GetPages(), element); - auto page2Ref = source1.LoadPage(RPageStorage::ColumnHandle_t{}, NTupleSize_t{0}); - EXPECT_EQ( - 0, memcmp(page2Ref.Get().GetBuffer(), "\x00\x00\x00\x00\x04\x00\x00\x00\x08\x00\x00\x00\x0c\x00\x00\x00", 16)); -#endif -}