[PATCH v5 1/1] x86-64: Add vector acos/acosf implementation to libmvec

[Noah Goldstein]

On Mon, Dec 20, 2021 at 1:20 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
> On Mon, Dec 20, 2021 at 10:08 AM Sunil Pandey <skpgkp2@gmail.com> wrote:
> >
> > On Sun, Dec 19, 2021 at 12:42 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > >
> > > On Sun, Dec 19, 2021 at 2:26 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > >
> > > > On Sun, Dec 19, 2021 at 12:29:07PM -0600, GNU C Library wrote:
> > > > > On Sun, Dec 19, 2021 at 11:19 AM Sunil K Pandey via Libc-alpha
> > > > > <libc-alpha@sourceware.org> wrote:
> > > > > >
> > > > > > Implement vectorized acos/acosf containing SSE, AVX, AVX2 and
> > > > > > AVX512 versions for libmvec as per vector ABI.  It also contains
> > > > > > accuracy and ABI tests for vector acos/acosf with regenerated ulps.
> > > > > > ---
> > > > >
> > > > > Have a few small comments but generally okay with a patch like this
> > > > > one going out in
> > > > > 2.35.
> > > >
> > > > ...
> > > >
> > > > >
> > > > > > +#define poly_coeff_6                   896
> > > > > > +#define poly_coeff_7                   960
> > > > > > +#define poly_coeff_8                   1024
> > > > > > +#define poly_coeff_9                   1088
> > > > > > +#define poly_coeff_10                  1152
> > > > > > +#define poly_coeff_11                  1216
> > > > > > +#define poly_coeff_12                  1280
> > > > > > +#define PiH                            1344
> > > > > > +#define Pi2H                           1408
> > > > >
> > > > > There is enough memory here it may pay to make the accesses
> > > >
> > > > Did you enough registers?
> > >
> > > This shouldn't affect register allocation.
> > > It's just if in the program we access: poly_coeff_11 -> poly_coeff_6
> > > -> poly_coeff_8
> > >
> > > it might be beneficial to organize the addresses of 11/6/8 s.t its
> > > sequential memory
> > > accesses from the table i.e
> > > #define poly_coeff_11 896
> > > #define poly_coeff_6 960
> > > #define poly_coeff_8 1024
> > > ...
> > >
> > > Random example and just a thought. Figure if coming in cold it might
> > > save a cache miss or two because it has an easy to recognize pattern
> > > for the HW prefetcher. Don't think it's make or break.
> > >
> >
> > Good suggestion. It's difficult to hand modify. Will let compiler team know
> > about this optimization.
>
> Like I said, can live with/without this optimization in the first
> version (mostly
> because I think its unclear what the actual best schema is), but this patch
> is being submitted as asm and meant to be maintained as asm. If the
> only feasible
> way to make future changes/optimizations is to update the compiler and
> recompile
> some higher level language, that's an issue.
>
> >
> > > >
> > > > > sequential in memory.
> > > >
> > > > This is based on Intel compiler generated codes.  We will evaluate
> > > > Intel compiler changes.
> > > >
> > > > ...
> > > >
> > > > > > +
> > > > > > +#include <sysdep.h>
> > > > > > +        vfmadd231pd {rn-sae}, %zmm3, %zmm11, %zmm10
> > > > > > +        andl      %eax, %ecx
> > > > > drop I think
> > > > >
> > > > > > +        vmovups   poly_coeff_12+__svml_dacos_data_internal(%rip), %zmm11
> > > > > > +        kmovw     %ecx, %k3
> > > > > kandw %k4, %k2, %k3
> > > >
> > > > This may not be faster since mask register can only go to port 0.  We
> > > > will evaluate register allocation in Intel compiler.
> > >
> > > `kmovw` and `kandw` are both 1uop port0.
> > >
> > > `andl` + `kmovw` is 2 uops and has 4c latency
> > > vs
> > > `kandw` is 1 uop and 1c latency.
> >
> > Will be fixed in v6.

In the other patches (for other functions, this one is fine) can you
have the compiler printout (maybe just a comment at the end of the line)
the the live-intervals for each register assignment. Looking at this code
there is a perception of extreme register pressure but a lot of that seems
forced by suspect instruction scheduling. It would be easier to notice that
for future maintenance with the the comment.
> >
> > > >
> > > >
> > > > Thanks.
> > > >
> > > > H.J.