Re: define pg_structiszero(addr, s, r)

Hi,

On Mon, Nov 11, 2024 at 06:19:50AM +0000, Bertrand Drouvot wrote:
> Hi,
>
> On Sat, Nov 09, 2024 at 04:15:04AM +0000, Bertrand Drouvot wrote:
> > Hi,
> >
> > On Fri, Nov 08, 2024 at 11:18:09PM +1300, David Rowley wrote:
> > > I tried with [1] and the
> > > latest gcc does not seem to be smart enough to figure this out. I
> > > tried adding some additional len checks that the compiler can use as a
> > > cue and won't need to emit code for the checks providing the function
> > > does get inlined. That was enough to get the compiler to not emit the
> > > loops when they'll not be used. See the -DCHECK_LEN flag I'm passing
> > > in the 2nd compiler window. I just don't know if putting something
> > > like that into the code is a good idea as if the function wasn't
> > > inlined for some reason, the extra len checks would have to be
> > > compiled into the function.
> > >
> > > David
> > >
> > > [1] https://godbolt.org/z/xa81ro8GK
> >
> > Looking at it, that looks like an issue.
> >
> > I mean, without the -DCHECK_LEN flag then the SIMD code will read up to 48 bytes
> > beyond the struct's memory (which is 16 bytes):
> >
> > This is fine:
> > "
> > movdqu xmm0, XMMWORD PTR [rdi]
> > "
> >
> > But I don't think it is:
> >
> > "
> > movdqu xmm2, XMMWORD PTR [rdi+16]
> > movdqu xmm1, XMMWORD PTR [rdi+32]
> > movdqu xmm3, XMMWORD PTR [rdi+48]
> > "
> >
> > given that the struct size is only 16 bytes.
> >
> > Thoughts?
>
> What about "simply" starting pg_memory_is_all_zeros() with?
>
> "
> if (len < sizeof(size_t)*8) {
> while (p < end) {
> if (*p++ != 0)
> return false;
> }
> return true;
> }
> "
>
> That way:
>
> - we prevents reading beyond the memory area in the SIMD section (if < 64 bytes)
> - we make sure that aligned_end can not be after the real end (could be if the
> len is < 8 bytes)
> - there is no need for additional size checks later in the code
> - len < 64 bytes will be read byte per byte but that's likely "enough" (if not
> faster) for those "small" sizes
>

To handle the "what about the len check if the function is not inlined?", I
can't think about a good approach.

I thought about using a macro like:

"
#define pg_memory_is_all_zeros(ptr, len) \
((len) < sizeof(size_t) * 8 ? \
pg_memory_is_all_zeros_small((ptr), (len)) : \
pg_memory_is_all_zeros_large((ptr), (len)))
"

but that would not help (as the len check would need to be done too, so same
run time cost).

I also thought about using __builtin_constant_p(len) but that would not help
because still we need the len check for safety.

So please find attached v11 that uses "only" one len check into the function to
ensure that we won't read beyond the memory area (should its size be < 64 or < 8).

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com