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

[Carlos O'Donell <carlos@redhat.com>]

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.

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

Missing "for BE" ?

> +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

> +   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

> +   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

> +   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

> +   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).

> +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
>