Re: [PATCH v5 2/2] x86: Optimize strlen-avx2.S

[Sunil Pandey <skpgkp2@gmail.com>]

On Wed, Oct 5, 2022 at 10:11 AM Aurelien Jarno <aurelien@aurel32.net> wrote:
>
> On 2022-10-04 18:10, Sunil Pandey via Libc-alpha wrote:
> > On Tue, Oct 4, 2022 at 2:20 PM Aurelien Jarno <aurelien@aurel32.net> wrote:
> > >
> > > On 2022-09-28 06:54, Sunil Pandey via Libc-stable wrote:
> > > > Attached patch fixes BZ# 29611.
> > > >
> > > > I would like to backport it to 2.32,2.31,2.30,2.29 and 2.29. Let me know
> > > > if there is any objection.
> > >
> > > Sorry to be late on this. I have a few comments about that patch:
> > >
> > > > From 86e1d88e1a3c126597ef39165275ada7564cfce9 Mon Sep 17 00:00:00 2001
> > > > From: "H.J. Lu" <hjl.tools@gmail.com>
> > > > Date: Mon, 19 Apr 2021 10:45:07 -0700
> > > > Subject: [PATCH] x86-64: Require BMI2 for strchr-avx2.S
> > > >
> > > > Since strchr-avx2.S updated by
> > > >
> > > > commit 1f745ecc2109890886b161d4791e1406fdfc29b8
> > > > Author: noah <goldstein.w.n@gmail.com>
> > > > Date:   Wed Feb 3 00:38:59 2021 -0500
> > > >
> > > >     x86-64: Refactor and improve performance of strchr-avx2.S
> > > >
> > > > uses sarx:
> > > >
> > > > c4 e2 72 f7 c0        sarx   %ecx,%eax,%eax
> > > >
> > > > for strchr-avx2 family functions, require BMI2 in ifunc-impl-list.c and
> > > > ifunc-avx2.h.
> > > >
> > > > (cherry picked from commit 83c5b368226c34a2f0a5287df40fc290b2b34359)
> > > > ---
> > > >  sysdeps/x86_64/multiarch/ifunc-avx2.h      |  4 ++--
> > > >  sysdeps/x86_64/multiarch/ifunc-impl-list.c | 12 +++++++++---
> > > >  2 files changed, 11 insertions(+), 5 deletions(-)
> > >
> > > First of all 1f745ecc2109890886b161d4791e1406fdfc29b8 never got
> > > backported to 2.32 and older branches, and strchr-avx2.S in those
> > > branches do not use BMI2 instructions. So it doesn't make sense to
> > > backport it.
> > >
> > > That said the change in ifunc-avx2.h fixes:
> > >
> > > - memchr and rawmemchr, broken by the backport of acfd088a1963 ("x86:
> > >   Optimize memchr-avx2.S")
> > > - strlen and strnlen, broken by the backport of aaa23c350715 ("x86:
> > >   Optimize strlen-avx2.S")
> > >
> > > So the issues are fixed, but mostly by chance.
> >
> > How do you know it is a "by chance" fix, do you have any evidence to back
> > your claim?
>
> My point is that the commit that has been backported is fixing a bug
> that doesn't exist in 2.32 branches. strchr-avx2.S does not the sarx
> instruction as the commit claims, and does not use other BMI2
> instructions either.
>
> However following the backport of commit acfd088a1963 and aaa23c350715
> in these branches, memchr-avx2.S and strlen-avx2.S use BMI2
> instructions, and as they use ifunc-avx2.h, this actually fixes the bug.
>

This patch got selected because it fixes the ifunc-avx2.h file. My preference
 is to take an existing patch if possible, rather than creating a new one for
 branches.

You are right, the original patch should have been composed differently to
make it crystal clear.

For backporting it's preferable to have small independent patches with
logical grouping.


> --
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net                 http://www.aurel32.net