Re: [PATCH v10 1/4] hash: pack the hitmask for hash in bulk lookup

[Thomas Monjalon <thomas@monjalon.net>]

05/07/2024 19:43, Yoan Picchi:
> On 7/4/24 21:31, David Marchand wrote:
> > On Wed, Jul 3, 2024 at 7:13 PM Yoan Picchi <yoan.picchi@arm.com> wrote:
> >> --- /dev/null
> >> +++ b/lib/hash/compare_signatures_arm_pvt.h
> > 
> > I guess pvt stands for private.
> > No need for such suffix, this header won't be exported in any case.
> 
> pvt do stand for private, yes. I had a look at the other lib and what 
> they used to state a header as private. Several (rcu, ring and stack) 
> use _pvt so it looks like that's might be the standard? If no, then how 
> am I supposed to differentiate a public and a private header?

Public headers are prefixed with rte_
We should not use _pvt


> >> +#if RTE_HASH_BUCKET_ENTRIES <= 8
> >> +       case RTE_HASH_COMPARE_NEON: {
> >> +               uint16x8_t vmat, vsig, x;
> >> +               int16x8_t shift = {0, 1, 2, 3, 4, 5, 6, 7};
> >> +               uint16_t low, high;
> >> +
> >> +               vsig = vld1q_dup_u16((uint16_t const *)&sig);
> >> +               /* Compare all signatures in the primary bucket */
> >> +               vmat = vceqq_u16(vsig, vld1q_u16((uint16_t const *)prim_bucket_sigs));
> >> +               x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)), shift);
> >> +               low = (uint16_t)(vaddvq_u16(x));
> >> +               /* Compare all signatures in the secondary bucket */
> >> +               vmat = vceqq_u16(vsig, vld1q_u16((uint16_t const *)sec_bucket_sigs));
> >> +               x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)), shift);
> >> +               high = (uint16_t)(vaddvq_u16(x));
> >> +               *hitmask_buffer = low | high << RTE_HASH_BUCKET_ENTRIES;
> >> +
> >> +               }
> >> +               break;
> >> +#endif
> >> +       default:
> >> +               for (unsigned int i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) {
> >> +                       *hitmask_buffer |= (sig == prim_bucket_sigs[i]) << i;
> >> +                       *hitmask_buffer |=
> >> +                               ((sig == sec_bucket_sigs[i]) << i) << RTE_HASH_BUCKET_ENTRIES;
> >> +               }
> >> +       }
> >> +}
> > 
> > IIRC, this code is copied in all three headers.
> > It is a common scalar version, so the ARM code could simply call the
> > "generic" implementation rather than copy/paste.
> 
> Out of the three files, only two versions are the same: generic and arm. 
> Intel's version do have some padding added (given it's sparse).
> I prefer to keep a scalar version in the arm implementation because 
> that's what match the legacy implementation. We used to be able to 
> choose (at runtime) to use the scalar path even if we had neon. In 
> practice the choice ends up being made from #defines, but as far as this 
> function goes, it is a runtime decision.

I have no strong opinion.