Updated implementation of Compress ops#3166
Conversation
jan-wassenberg
left a comment
jan-wassenberg
left a comment
There was a problem hiding this comment.
Excellent, thanks for tackling this duplication.
Some suggestions, happy to discuss:
| template <class D> | ||
| static constexpr bool NeedToSplitForCompress8Or16() { | ||
| return (HWY_MAX_LANES_D(D) * sizeof(TFromD<D>)) >= | ||
| ((HWY_TARGET == HWY_SVE_256) ? 32 : 16) && |
There was a problem hiding this comment.
Should this be HWY_MIN_BYTES? It is possible we later add a 512-bit SVE target.
| using TW = | ||
| If<(sizeof(T) == 1 && | ||
| ((HWY_POW2_D(D) <= -2) || | ||
| (HWY_MAX_LANES_D(D) <= ((HWY_TARGET == HWY_SVE_256) ? 8 : 4)))), |
| template <class V, HWY_IF_T_SIZE_V(V, 8)> | ||
| HWY_API V CompressNot(V v, svbool_t mask) { | ||
| #if HWY_TARGET == HWY_SVE2_128 || HWY_IDE | ||
| #if HWY_TARGET == HWY_SVE2_128 |
There was a problem hiding this comment.
FYI the HWY_IDE is to prevent our IDE from greying out the code. Not a big deal for short code sections, but still nice to keep. The endif/if instead of elif is also for this purpose, so that multiple blocks are not greyed out.
| return kCompressNot64x2Table; | ||
| } | ||
|
|
||
| #if HWY_CAP_GE256 |
There was a problem hiding this comment.
This one is deprecated, how about HWY_MIN_BYTES >= 32?
| // is likely more costly than the higher cache footprint from storing bytes. | ||
| const uint8_t (&table)[2048] = detail::GetCompress16x8Table(); | ||
|
|
||
| #if HWY_IS_BIG_ENDIAN |
There was a problem hiding this comment.
Would you like to add/use a constexpr NativeFromLittleEndian (or similar name) function to reduce #if?
| } | ||
| } | ||
|
|
||
| #if HWY_CAP_GE256 |
| const size_t count = PopCount(mask_bits); | ||
|
|
||
| const auto compressed = Compress(v, m); | ||
| #if (HWY_ARCH_S390X && HWY_TARGET <= HWY_Z14) || \ |
There was a problem hiding this comment.
Should we also add a HWY_NATIVE_STOREN (like HWY_NATIVE_MASK) for this?
| #define HWY_NATIVE_FMA 1 | ||
| #define HWY_NATIVE_DOT_BF16 0 | ||
| #define HWY_NATIVE_MASK 1 | ||
| #define HWY_HAVE_NATIVE_STORE_N 1 |
There was a problem hiding this comment.
One more thing: how about we rename HWY_HAVE_NATIVE_STORE_N to HWY_NATIVE_STORE_N ? Currently, HWY_HAVE_ is for "are these available at all, e.g. float64", whereas HWY_NATIVE_MASK etc mean "it's available everywhere, but is it efficient"? This new flag belongs to the latter category, so good to keep it consistent with naming.
|
We had an internal user calling the detail:: functions, causing internal CI to fail. I will fix that soon. |
2 participants
Resolves one of the issues raised in issue #3142.
Moved the default implementation of the Compress ops for vectors with 16-bit, 32-bit, or 64-bit lane types into generic_ops-inl.h.
Added new
Compress(d, v, m)andCompressNot(d, v, m)overloads that take an additionaldparameter as the additionaldparameter allows the SVE implementation of I8/U8/I16/U16/F16/BF16Compress(d, v, m)to avoid having to splitvinto halves ifLanes(d) <= Lanes(RepartitionToWide<RebindToUnsigned<DFromV<decltype(v)>>>())is known to be true at compile time.Added new
CompressBits(d, v, bits)overload that takes an additionaldparameter, which allows theCompressBits(d, v, bits)overload to avoid having to load more thanHWY_MAX(Lanes(d), 8)mask bits ifLanes(d) < Lanes(DFromV<decltype(v)>())is true, which is possible on RVV and SVE targets.In addition, it is also possible for
Lanes(DFromV<decltype(v)>())to be greater than 64 on RVV and SVE targets (other than SVE_256 or SVE2_128), and LoadMaskBits only requires that 8 bytes ofbitsbe readable ifLanes(d) <= 64is true.Also reimplemented I8/U8/I16/U16/F16/BF16
Compress(d, v, m)for SVE targets to avoid unnecessary splitting ofvifLanes(d) <= Lanes(RepartitionToWide<RebindToUnsigned<DFromV<decltype(v)>>>())is known to be true at compile time.The new I8/U8 Compress implementation in arm_sve-inl.h also avoids using temporary arrays to store
vand the bits ofmas SVE can efficiently combine the results of compressing the two halves ofvusing detail::Splice.Also updated emu128-inl.h to use the implementation of Compress in emu128-inl.h for I8/U8 vectors as the implementation in emu128-inl.h is more efficient than the default implementation in generic_ops-inl.h for I8/U8 vectors.
Also updated
CompressIsPartition<T>to make CompressIsPartition::value equal to 1 for all lane types on the EMU128 target as the EMU128 implementation of Compress is known to be a partitioning compress.