Re: [PATCH 05/42] x86-64: Add vector asin/asinf implementation to libmvec

[Sunil Pandey <skpgkp2@gmail.com>]

On Wed, Nov 24, 2021 at 2:52 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Wed, 24 Nov 2021, Sunil K Pandey via Libc-alpha wrote:
>
> > Implement vectorized asin/asinf containing SSE, AVX, AVX2 and
> > AVX512 versions for libmvec as per vector ABI.  It also contains
> > accuracy and ABI tests for vector asin/asinf with regenerated ulps.
>
> Here are some general comments that probably also apply to other patches
> in the series; you'll need to review all the other patches in the series
> for such issues.
>
> >  sysdeps/x86/fpu/bits/math-vector.h            |    4 +
>
> I'd expect corresponding changes to
> sysdeps/x86/fpu/finclude/math-vector-fortran.h so the functions can be
> used from Fortran code.

Function added in sysdeps/x86/fpu/finclude/math-vector-fortran.h  in v2 patch.

>
>
> > +        cmpltpd   128+__svml_dasin_data_internal(%rip), %xmm0
> > +        cvtps2pd  %xmm1, %xmm2
> > +        movups    256+__svml_dasin_data_internal(%rip), %xmm6
>
> Please see my comments from the review of the original libmvec addition
> regarding how to make the code and the tables of data it uses more
> readable
> <https://sourceware.org/legacy-ml/libc-alpha/2014-10/msg00324.html>.
>
> We don't want hardcoded offsets into data tables, such as 128 or 256 here,
> in the function implementations, and we want the data tables to have
> meaningful names or comments on each part of the table saying what the
> semantics are.  That might mean defining lots of separate smaller tables,
> each with an appropriate name and comment describing its semantics, rather
> than just the one __svml_dasin_data_internal.  Or, if it's desirable in
> some cases to load the table address once and do everything else based on
> offsets from it, it might mean defining C macros (with meaningful names)
> for the various offsets, like those macros in svml_d_trig_data.h, and then
> defining the data using corresponding assembler macros to verify that the
> offsets for the data in the table actually match those in the C macros and
> cause a compile-time error if the C macros don't match the offsets.

We have a plan to address  the data table issue by adding comments and macro.
It will describe what they are. But we do not have time to finish for
2.35 and we
will fix it in 2.36.

>
> > +        .cfi_escape 0x10, 0x19, 0x0e, 0x38, 0x1c, 0x0d, 0xc0, 0xff, 0xff, 0xff, 0x1a, 0x0d, 0x70, 0xff, 0xff, 0xff, 0x22
> > +        .cfi_escape 0x10, 0x1f, 0x0e, 0x38, 0x1c, 0x0d, 0xc0, 0xff, 0xff, 0xff, 0x1a, 0x0d, 0xa0, 0xff, 0xff, 0xff, 0x22
> > +        .cfi_escape 0x10, 0x20, 0x0e, 0x38, 0x1c, 0x0d, 0xc0, 0xff, 0xff, 0xff, 0x1a, 0x0d, 0x90, 0xff, 0xff, 0xff, 0x22
>
> Why do you have all these .cfi_escape in the .S sources?
>

.cfi_escape is a standard way to define DW_CFA_expression and associated
 registers. For example

DW_CFA_expression: r4 (rsi) (DW_OP_lit8; DW_OP_minus; DW_OP_const4s:
-64; DW_OP_and; DW_OP_const4s: -200; DW_OP_plus)

> If something needs to be represented in unwind info, it would be better to
> do it with more reader-friendly directives rather than .cfi_escape.  If
> such reader-friendly directives require a newer binutils version than the
> current minimum, what version do they require?  We could consider updating
> that minimum.
>
> > +.LBL_1_3:
>
> I don't think this sort of label name is very friendly.  At least, I'd
> suggest that each such label should have a comment explaining what the
> code is doing at that point, to help readers follow the flow of control in
> the function.
>
> In general, it helps for long assembly sources (and these are certainly
> long) to be heavily commented to help readers follow what is happening
> where in the code.  While a comment per line of C code would be heavily
> excessive in C sources, having comments in assembly code per every few
> lines of equivalent C seems quite reasonable.  That means many more
> comments than there are in this patch at present.

We will add more comments, but we need to add comments to the data table first.
We will do it in 2.36. For 2.35, we just changed the label to sequential.

I am going to send the v2 patch today.

> --
> Joseph S. Myers
> joseph@codesourcery.com