Re: [PATCH v12 03/31] Add string vectorized find and detection functions

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 02/02/23 21:24, Richard Henderson wrote:
> On 2/2/23 08:11, Adhemerval Zanella wrote:
>> +/* With similar caveats, identify zero bytes in X1 and bytes that are
>> +   not equal between in X1 and X2.  */
>> +static __always_inline find_t
>> +find_zero_ne_low (op_t x1, op_t x2)
>> +{
>> +  return (~find_zero_eq_low (x1, x2)) + 1;
>> +}
> 
> This is no longer used, and suspected buggy.  Duh, I now see why it's buggy -- it's inverting both zero comparison and eq comparison -- we wanted to invert only eq.
> 
> Let's remove this rather than attempting to fix.  I suspect it'll work out very similar to our existing find_zero_ne_all().

Ack, we haven't see any issue because it not really use anywhere.

> 
> 
>> +/* Similarly, but perform the search for byte equality between X1 and X2.  */
>> +static __always_inline unsigned int
>> +index_first_zero (op_t x1, op_t x2)
>> +{
>> +  if (__BYTE_ORDER == __LITTLE_ENDIAN)
>> +    x1 = find_zero_low (x1, x2);
>> +  else
>> +    x1 = find_zero_all (x1, x2);
>> +  return index_first (x1);
>> +}
> ...
>> +/* Similarly, but search for the last zero within X.  */
>> +static __always_inline unsigned int
>> +index_last_zero (op_t x)
>> +{
>> +  return index_last (find_zero_all (x));
>> +}
> 
> Why did this lose the __BYTE_ORDER test?  It should be the inverse of index_first_zero.

I think because find_zero_all works for both LE and BE.  But we can optimize it
slight for BE:

static __always_inline unsigned int
index_last_zero (op_t x)
{
  if (__BYTE_ORDER == __LITTLE_ENDIAN)
    x = find_zero_all (x);
  else
    x = find_zero_low (x);
  return index_last (x);
}


> 
> 
>> +static __always_inline unsigned int
>> +index_last_eq (op_t x1, op_t x2)
>> +{
>> +  return index_last_zero (x1 ^ x2);
>> +}

I think it should not be required with index_last_zero using __BYTE_ORDER
test as above.