Re: [PATCH v5 03/17] Add string-maskoff.h generic header

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

On 20/09/22 08:43, Carlos O'Donell wrote:
> On Mon, Sep 19, 2022 at 04:59:06PM -0300, Adhemerval Zanella via Libc-alpha wrote:
>> Macros to operate on unaligned access for string operations:
>>
>>   - create_mask: create a mask based on pointer alignment to sets up
>>     non-zero bytes before the beginning of the word so a following
>>     operation (such as find zero) might ignore these bytes.
>>
>>   - highbit_mask: create a mask with high bit of each byte being 1,
>>     and the low 7 bits being all the opposite of the input.
> 
> I really appreciate the effort you've put into documenting the purpose
> of each function! It's really awesome to reach such nice coments. Thank
> you for that. I've gone through this to review the implementation and
> the descriptions. I think it needs a little more tweaking.
> 
>> These macros are meant to be used on optimized vectorized string
>> implementations.
>> ---
>>  sysdeps/generic/string-maskoff.h | 73 ++++++++++++++++++++++++++++++++
>>  1 file changed, 73 insertions(+)
>>  create mode 100644 sysdeps/generic/string-maskoff.h
>>
>> diff --git a/sysdeps/generic/string-maskoff.h b/sysdeps/generic/string-maskoff.h
>> new file mode 100644
>> index 0000000000..831647bda6
>> --- /dev/null
>> +++ b/sysdeps/generic/string-maskoff.h
>> @@ -0,0 +1,73 @@
>> +/* Mask off bits.  Generic C version.
>> +   Copyright (C) 2022 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <http://www.gnu.org/licenses/>.  */
>> +
>> +#ifndef _STRING_MASKOFF_H
>> +#define _STRING_MASKOFF_H 1
>> +
>> +#include <endian.h>
>> +#include <limits.h>
>> +#include <stdint.h>
>> +#include <string-optype.h>
>> +
>> +/* Provide a mask based on the pointer alignment that sets up non-zero
>> +   bytes before the beginning of the word.  It is used to mask off
>> +   undesirable bits from an aligned read from an unaligned pointer.
>> +   For instance, on a 64 bits machine with a pointer alignment of
> 
> s/bits/-bit/g
> 
> While it is technically correct English to say "A 64-bits machine", this
> is not the normative usage.
> 
> I suggest we use the normative "64-bit machine." We can talk about 64
> bits, and alignment as bits etc.

Alright.

> 
>> +   3 the function returns 0x0000000000ffffff for LE and 0xffffff0000000000
>> +   (meaning to mask off the initial 3 bytes).  */
> 
> Missing "for BE" ?

Ack.

> 
>> +static inline op_t
>> +create_mask (uintptr_t i)
>> +{
>> +  i = i % sizeof (op_t);
> 
> OK. Wrap the value.
> 
>> +  if (__BYTE_ORDER == __LITTLE_ENDIAN)
>> +    return ~(((op_t)-1) << (i * CHAR_BIT));
>> +  else
>> +    return ~(((op_t)-1) >> (i * CHAR_BIT));
> 
> OK. Shift.
> 
>> +}
>> +
>> +/* Setup an word with each byte being c_in.  For instance, on a 64 bits
> 
> s/an/a/g
> s/bits/-bit/g

Ack.

> 
>> +   machine with input as 0xce the functions returns 0xcececececececece.  */
>> +static inline op_t
>> +repeat_bytes (unsigned char c_in)
>> +{
>> +  return ((op_t)-1 / 0xff) * c_in;
>> +}
> 
> How does the compiler do here on the various architectures to produce
> the deposit/expand instructions that could be used for this operation?
> 
> aarch64 gcc trunk:
>         ldrb    r3, [r7, #7]    @ zero_extendqisi2
>         mov     r2, #16843009
>         mul     r3, r2, r3
> 
> x86_64 gcc trunk:
>         movzx   eax, BYTE PTR [rbp-4]
>         imul    eax, eax, 16843009
> 
> s390x gcc12:
> 	ic      %r1,167(%r11)
>         lhi     %r2,255
>         nr      %r1,%r2
>         ms      %r1,.L4-.L3(%r5)
>         llgfr   %r1,%r1
> 
> Looks OK, and the static inline will get optimized with the rest of
> the operations.
> 
>> +
>> +/* Based on mask created by 'create_mask', mask off the high bit of each
> 
> s/on/on a/g

Ack.

> 
>> +   byte in the mask.  It is used to mask off undesirable bits from an
>> +   aligned read from an unaligned pointer, and also taking care to avoid
> 
> s/and/while/g

Ack.

> 
>> +   match possible bytes meant to be matched.  For instance, on a 64 bits
> 
> Suggest:
> matching possible bytes not meant to be matched.
> 
> s/bits/-bits/g

Ack.

> 
>> +   machine with a mask created from a pointer with an alignment of 3
>> +   (0x0000000000ffffff) the function returns 0x7f7f7f0000000000 for BE
>> +   and 0x00000000007f7f7f for LE.  */
>> +static inline op_t
>> +highbit_mask (op_t m)
>> +{
>> +  return m & repeat_bytes (0x7f);
> 
> OK.
> 
>> +}
>> +
>> +/* Return the address of the op_t word containing the address P.  For
>> +   instance on address 0x0011223344556677 and op_t with size of 8,
>> +   it returns 0x0011223344556670.  */
> 
> Could you expand on this a bit more? It's a bit opaque what we might use
> this for (I have some ideas).

Maybe: 

/* Return the word aligned address containing the address P.  For instance for
   the address 0x0011223344556677 with op_t with size of 8, it returns
   0x0011223344556670.  */

> 
>> +static inline op_t *
>> +word_containing (char const *p)
>> +{
>> +  return (op_t *) (p - (uintptr_t) p % sizeof (op_t));
>> +}
>> +
>> +#endif /* _STRING_MASKOFF_H  */
>> -- 
>> 2.34.1
>>
>