On Thu, Oct 9, 2025 at 10:11 PM Bruce Richardson wrote: > On Wed, Jul 16, 2025 at 04:04:39PM +0530, Shreesh Adiga wrote: > > Replace the clearing of lower 32 bits of XMM register with blend of > > zero register. > > Replace the clearing of upper 64 bits of XMM register with > _mm_move_epi64. > > Clang is able to optimize away the AND + memory operand with the > > above sequence, however GCC is still emitting the code for AND with > > memory operands which is being explicitly eliminated here. > > > > Additionally replace the 48 byte crc_xmm_shift_tab with the contents of > > shf_table which is 32 bytes, achieving the same functionality. > > > > Signed-off-by: Shreesh Adiga <16567adigashreesh@gmail.com> > > --- > > lib/net/net_crc_sse.c | 30 +++++++----------------------- > > 1 file changed, 7 insertions(+), 23 deletions(-) > > > > See inline below. Changes to the reduce_64_to_32 look ok, I don't know > enough to understand fully the other changes you made. Maybe split the > patch into two patches for review and merge separately? > > /Bruce > > > diff --git a/lib/net/net_crc_sse.c b/lib/net/net_crc_sse.c > > index 112dc94ac1..eec854e587 100644 > > --- a/lib/net/net_crc_sse.c > > +++ b/lib/net/net_crc_sse.c > > @@ -96,20 +96,13 @@ crcr32_reduce_128_to_64(__m128i data128, __m128i > precomp) > > static __rte_always_inline uint32_t > > crcr32_reduce_64_to_32(__m128i data64, __m128i precomp) > > { > > - static const alignas(16) uint32_t mask1[4] = { > > - 0xffffffff, 0xffffffff, 0x00000000, 0x00000000 > > - }; > > - > > - static const alignas(16) uint32_t mask2[4] = { > > - 0x00000000, 0xffffffff, 0xffffffff, 0xffffffff > > - }; > > __m128i tmp0, tmp1, tmp2; > > > > - tmp0 = _mm_and_si128(data64, _mm_load_si128((const __m128i > *)mask2)); > > + tmp0 = _mm_blend_epi16(_mm_setzero_si128(), data64, 252); > > Minor nit: 252 would be better in hex to make it clearer that it's the > lower two bits are unset. Even better, how about switching the operands so > that the constant is just "3", which is clearer again. > Okay I will update it with your suggestion in the next patch. > > > > tmp1 = _mm_clmulepi64_si128(tmp0, precomp, 0x00); > > tmp1 = _mm_xor_si128(tmp1, tmp0); > > - tmp1 = _mm_and_si128(tmp1, _mm_load_si128((const __m128i *)mask1)); > > + tmp1 = _mm_move_epi64(tmp1); > > > > This change LGTM. > > > tmp2 = _mm_clmulepi64_si128(tmp1, precomp, 0x10); > > tmp2 = _mm_xor_si128(tmp2, tmp1); > > @@ -118,13 +111,11 @@ crcr32_reduce_64_to_32(__m128i data64, __m128i > precomp) > > return _mm_extract_epi32(tmp2, 2); > > } > > > > -static const alignas(16) uint8_t crc_xmm_shift_tab[48] = { > > - 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > - 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > +static const alignas(16) uint8_t crc_xmm_shift_tab[32] = { > > + 0x00, 0x81, 0x82, 0x83, 0x84, 0x85, 0x86, 0x87, > > + 0x88, 0x89, 0x8a, 0x8b, 0x8c, 0x8d, 0x8e, 0x8f, > > 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, > > - 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, > > - 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > - 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff > > + 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f > > }; > > > > Can you perhaps explain how changing this table doesn't break existing uses > of the table as it now is in the code? Specifically, does xmm_shift_left > function not now have different behaviour? > Sure, crc_xmm_shift_tab is only used inside xmm_shift_left which is only used when the total data_len is < 16. We call xmm_shift_left(fold, 8 - data_len) when len <= 4 and xmm_shift_left(fold, 16 - data_len) when 5 <= len <= 15. This results in accessing crc_xmm_shift_tab between 1 and 31, i.e. element 0 is never accessed. Now if we take a specific case of xmm_shift_left(fold, 10), then previously the shuffle register would get loaded with (crc_xmm_shift_tab + 16 - 10) which would be crc_xmm_shift_tab + 6: {0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05} which when used with PSHUFB would result in first 10 bytes of reg being 0 and the lower 6 elements moving left: {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, d1, d2, d3, d4, d5, d6}. The 0 get inserted because PSHUFB with index > 0x7f (MSB set) results in 0. Now since we have replaced the contents of crc_xmm_shift_tab with shf_table, we will load: {0x86, 0x87, 0x88, 0x89, 0x8a, 0x8b, 0x8c, 0x8d, 0x8e, 0x8f, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05} into the index register. Since the first 10 elements have MSB set, PSHUFB will again result in the same vector above with first 10 elements being zeroes and the 6 moving left as intended. Since xmm_shift_left is called with num between 11 and 1, we don't access crc_xmm_shift_tab[0] and the remaining elements 0x8{i} behave identically to 0xff when used with PSHUFB. Thus the xmm_shift_left behavior for currently used num values inside this file has identical behavior. > > > /** > > @@ -216,19 +207,12 @@ crc32_eth_calc_pclmulqdq( > > 0x80808080, 0x80808080, 0x80808080, 0x80808080 > > }; > > > > - const alignas(16) uint8_t shf_table[32] = { > > - 0x00, 0x81, 0x82, 0x83, 0x84, 0x85, 0x86, 0x87, > > - 0x88, 0x89, 0x8a, 0x8b, 0x8c, 0x8d, 0x8e, 0x8f, > > - 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, > > - 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f > > - }; > > - > > __m128i last16, a, b; > > > > last16 = _mm_loadu_si128((const __m128i *)&data[data_len - > 16]); > > > > temp = _mm_loadu_si128((const __m128i *) > > - &shf_table[data_len & 15]); > > + &crc_xmm_shift_tab[data_len & 15]); > > a = _mm_shuffle_epi8(fold, temp); > > > > temp = _mm_xor_si128(temp, > > -- > > 2.49.1 > > >