Re: [PATCH] mips: add single-instruction math functions

[Rich Felker <dalias@libc.org>]

On Fri, Sep 13, 2019 at 04:01:41PM -0400, Rich Felker wrote:
> On Fri, Sep 13, 2019 at 03:12:11PM -0400, James Y Knight wrote:
> > I've no idea about the issues with MIPS' instruction set, but yes, to be
> > correct, abs does need to affect only the sign bit, leaving everything else
> > the same.
> > 
> > It's specified as such in IEEE 754-2008: 5.5.1 Sign bit operations --
> > "Implementations shall provide the following homogeneous
> > quiet-computational sign bit operations for all supported arithmetic
> > formats; they only affect the sign bit. The operations treat floating-point
> > numbers and NaNs alike, and signal no exception. These operations may
> > propagate non-canonical encodings."
> > ....
> > "abs(x) copies a floating-point operand x to a destination in the same
> > format, setting the sign bit to 0 (positive)."
> > 
> > (That section also defines the copy, negate, and copySign operations)
> 
> OK, the text I was missing was:
> 
> "The fabs functions in <math.h> provide the abs function recommended
> in the Appendix to IEC 60559."
> 
> from F.3 ¶1.
> 
> > On Fri, Sep 13, 2019 at 2:57 PM Rich Felker <dalias@libc.org> wrote:
> > 
> > > On Fri, Sep 13, 2019 at 02:31:23PM -0400, Rich Felker wrote:
> > > > On Wed, Sep 11, 2019 at 01:05:04PM +0300, info@mobile-stream.com wrote:
> > > > >
> > > > > non-commit text:
> > > > > gcc puts annoying nop into the delay slot for these functions, e.g.:
> > > > >     abs.d   $f0,$f12
> > > > >     jr      $ra
> > > > >      nop
> > > > > is there any way to get rid of this without using pure .S?
> > > > >
> > > > >
> > > > >
> > > > > SQRT.fmt exists on MIPS II+ (float), MIPS III+ (double).
> > > > >
> > > > > ABS.fmt exists on MIPS I+ but only cores with ABS2008 flag in FCSR
> > > > > implement the required behaviour.
> > > > > ---
> > > > >  src/math/mips/fabs.c  | 16 ++++++++++++++++
> > > > >  src/math/mips/fabsf.c | 16 ++++++++++++++++
> > > > >  src/math/mips/sqrt.c  | 16 ++++++++++++++++
> > > > >  src/math/mips/sqrtf.c | 16 ++++++++++++++++
> > > > >  4 files changed, 64 insertions(+)
> > > > >  create mode 100644 src/math/mips/fabs.c
> > > > >  create mode 100644 src/math/mips/fabsf.c
> > > > >  create mode 100644 src/math/mips/sqrt.c
> > > > >  create mode 100644 src/math/mips/sqrtf.c
> > > > >
> > > > > diff --git a/src/math/mips/fabs.c b/src/math/mips/fabs.c
> > > > > new file mode 100644
> > > > > index 00000000..0a5aa3b1
> > > > > --- /dev/null
> > > > > +++ b/src/math/mips/fabs.c
> > > > > @@ -0,0 +1,16 @@
> > > > > +#if !defined(__mips_soft_float) && defined(__mips_abs2008)
> > > >
> > > > Why is this dependent on __mips_abs2008?
> > >
> > > OK, I see. The macro isn't well-documented, but it corresponds to the
> > > gcc -mabs=2008 option indicating an ISA variant that treats the
> > > abs.[sd] instruction as bitwise rather than arithmetic. I'm somewhat
> > > unclear on whether we should do anything like this; it's one of the
> > > places where MIPS blurred the distinction between ISA levels and
> > > incompatible ABIs, and seems to be able to produce code that would
> > > silently run on the wrong type of cpu and produce the wrong result (or
> > > maybe this is a runtime-switchable cpu mode?)
> > >
> > > I'm also unclear though whether fabs() actually needs to behave as
> > > bitwise. Neither C nor Annex F nor POSIX seems to specify its behavior
> > > for NANs except that a NAN is returned. Am I missing something? If
> > > not, it seems the 'legacy' abs.[sd] instructions would also be fine
> > > for implementing this, and then no conditional is needed.

Anyway, back to the patch review, I think the fabs[f] part should be
held back contingent on really understanding the ABI issue and whether
we can treat this as an ISA level and make it safe, or whether this is
really an incompatible ISA/ABI variant.

(And in any case, the caller will inline the insn and never call the
external fabs function if you're using the right CFLAGS...)

The sqrt part is probably okay for merge in the next release cycle.

Rich