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

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

On 05/01/23 20:26, Alejandro Colomar wrote:
> 
> 
> On 1/5/23 23:49, Noah Goldstein via Libc-alpha wrote:
>> On Mon, Sep 19, 2022 at 12:59 PM Adhemerval Zanella via Libc-alpha
>> <libc-alpha@sourceware.org> 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.
>>>
>>> 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
>>> +   3 the function returns 0x0000000000ffffff for LE and 0xffffff0000000000
>>> +   (meaning to mask off the initial 3 bytes).  */
>>> +static inline op_t
>>> +create_mask (uintptr_t i)
>>> +{
>>> +  i = i % sizeof (op_t);
>>> +  if (__BYTE_ORDER == __LITTLE_ENDIAN)
>>> +    return ~(((op_t)-1) << (i * CHAR_BIT));
>>> +  else
>>> +    return ~(((op_t)-1) >> (i * CHAR_BIT));
>>> +}
>>> +
>>> +/* Setup an word with each byte being c_in.  For instance, on a 64 bits
>>> +   machine with input as 0xce the functions returns 0xcececececececece.  */
>>> +static inline op_t
> 
> Hi Adhemerval and Noah,
> 
> I don't know what is the minimum C version for compiling glibc, but if you can ignore C89, I would propose something:
> 
> 'static inline' should be restricted to .c files, since if the compiler decides to not inline and you have it in a header, you end up with multiple static definitions for the same code.
> 
> In headers, I use C99 inline, which doesn't emit any object code when the compiler decides to not inline.  Then in a .c file, you add a prototype using 'extern inline', and the compiler will emit code there, exactly once.
> 
> Even if you have to support C89, I'd use [[gnu::always_inline]] together with 'static inline', to make sure that the compiler doesn't do nefarious stuff.

Although we build glibc with -std=gnu11 we also uses -fgnu89-inline, but regardless of
the inline mode 'multiple definitions' it is usually not an issue.  We do use
__always_inline in performant code, so it should be ok to use it here as well (although
I would expect that compiler will always inline these short functions).