[PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt

[PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt

[Keith Packard]

RISC-V has a couple of acceleration paths that are designed to repace
the general code for sqrt and fma.

The sqrt code was using the wrong filenames and ended up replacing the
wrong functions (sqrt instead of __ieee754_sqrt). For builds that
disabled errno, that code provided an alias for sqrt mapping to
__ieee754_sqrt so things "worked".

The fma code had the right filenames, but because it only generated
code on machines with hardware support, machines lacking it ended up
replacing the useful general implementation with an empty object file.

To fix this, I've renamed the RISC-V sqrt files then fixed both sqrt
and fma to #include the general code when the machine-specific code
wasn't used.

I tested the result by making sure every libm.a generated for RISC-V
contained exactly one of each relevant function.

[PATCH 1/2] libm/riscv: Fix machine-specific sqrt build process

[Keith Packard]

Like ARM, some RISC-V implementations have hardware sqrt. Support for
that can be detected at compile time, which the code did. However, the
filenames were incorrect so that both the risc-v specific and general
code were getting included in the resulting library.

Fix this by following the ARM model and #include'ing the general code
when the architecture-specific support is not available.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 newlib/libm/machine/riscv/Makefile.am         |  2 +-
 newlib/libm/machine/riscv/Makefile.in         | 22 +++++++++----------
 .../libm/machine/riscv/{s_sqrt.c => e_sqrt.c} |  6 ++---
 .../machine/riscv/{sf_sqrt.c => ef_sqrt.c}    |  6 ++---
 4 files changed, 16 insertions(+), 20 deletions(-)
 rename newlib/libm/machine/riscv/{s_sqrt.c => e_sqrt.c} (94%)
 rename newlib/libm/machine/riscv/{sf_sqrt.c => ef_sqrt.c} (94%)

diff --git a/newlib/libm/machine/riscv/Makefile.am b/newlib/libm/machine/riscv/Makefile.am
index a7783797a..870f2a7b6 100644
--- a/newlib/libm/machine/riscv/Makefile.am
+++ b/newlib/libm/machine/riscv/Makefile.am
@@ -7,7 +7,7 @@ LIB_SOURCES = \
 	feclearexcept.c fe_dfl_env.c fegetenv.c fegetexceptflag.c \
 	fegetround.c feholdexcept.c feraiseexcept.c fesetenv.c \
 	fesetexceptflag.c fesetround.c fetestexcept.c feupdateenv.c \
-	s_fma.c s_sqrt.c sf_fma.c sf_sqrt.c
+	s_fma.c e_sqrt.c sf_fma.c ef_sqrt.c
 
 noinst_LIBRARIES = lib.a
 lib_a_SOURCES = $(LIB_SOURCES)
diff --git a/newlib/libm/machine/riscv/Makefile.in b/newlib/libm/machine/riscv/Makefile.in
index c56830569..4ac1d30c4 100644
--- a/newlib/libm/machine/riscv/Makefile.in
+++ b/newlib/libm/machine/riscv/Makefile.in
@@ -77,8 +77,8 @@ am__objects_1 = lib_a-feclearexcept.$(OBJEXT) \
 	lib_a-fesetenv.$(OBJEXT) lib_a-fesetexceptflag.$(OBJEXT) \
 	lib_a-fesetround.$(OBJEXT) lib_a-fetestexcept.$(OBJEXT) \
 	lib_a-feupdateenv.$(OBJEXT) lib_a-s_fma.$(OBJEXT) \
-	lib_a-s_sqrt.$(OBJEXT) lib_a-sf_fma.$(OBJEXT) \
-	lib_a-sf_sqrt.$(OBJEXT)
+	lib_a-e_sqrt.$(OBJEXT) lib_a-sf_fma.$(OBJEXT) \
+	lib_a-ef_sqrt.$(OBJEXT)
 am_lib_a_OBJECTS = $(am__objects_1)
 lib_a_OBJECTS = $(am_lib_a_OBJECTS)
 DEFAULT_INCLUDES = -I.@am__isrc@
@@ -207,7 +207,7 @@ LIB_SOURCES = \
 	feclearexcept.c fe_dfl_env.c fegetenv.c fegetexceptflag.c \
 	fegetround.c feholdexcept.c feraiseexcept.c fesetenv.c \
 	fesetexceptflag.c fesetround.c fetestexcept.c feupdateenv.c \
-	s_fma.c s_sqrt.c sf_fma.c sf_sqrt.c
+	s_fma.c e_sqrt.c sf_fma.c ef_sqrt.c
 
 noinst_LIBRARIES = lib.a
 lib_a_SOURCES = $(LIB_SOURCES)
@@ -363,11 +363,11 @@ lib_a-s_fma.o: s_fma.c
 lib_a-s_fma.obj: s_fma.c
 	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-s_fma.obj `if test -f 's_fma.c'; then $(CYGPATH_W) 's_fma.c'; else $(CYGPATH_W) '$(srcdir)/s_fma.c'; fi`
 
-lib_a-s_sqrt.o: s_sqrt.c
-	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-s_sqrt.o `test -f 's_sqrt.c' || echo '$(srcdir)/'`s_sqrt.c
+lib_a-e_sqrt.o: e_sqrt.c
+	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-e_sqrt.o `test -f 'e_sqrt.c' || echo '$(srcdir)/'`e_sqrt.c
 
-lib_a-s_sqrt.obj: s_sqrt.c
-	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-s_sqrt.obj `if test -f 's_sqrt.c'; then $(CYGPATH_W) 's_sqrt.c'; else $(CYGPATH_W) '$(srcdir)/s_sqrt.c'; fi`
+lib_a-e_sqrt.obj: e_sqrt.c
+	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-e_sqrt.obj `if test -f 'e_sqrt.c'; then $(CYGPATH_W) 'e_sqrt.c'; else $(CYGPATH_W) '$(srcdir)/e_sqrt.c'; fi`
 
 lib_a-sf_fma.o: sf_fma.c
 	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-sf_fma.o `test -f 'sf_fma.c' || echo '$(srcdir)/'`sf_fma.c
@@ -375,11 +375,11 @@ lib_a-sf_fma.o: sf_fma.c
 lib_a-sf_fma.obj: sf_fma.c
 	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-sf_fma.obj `if test -f 'sf_fma.c'; then $(CYGPATH_W) 'sf_fma.c'; else $(CYGPATH_W) '$(srcdir)/sf_fma.c'; fi`
 
-lib_a-sf_sqrt.o: sf_sqrt.c
-	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-sf_sqrt.o `test -f 'sf_sqrt.c' || echo '$(srcdir)/'`sf_sqrt.c
+lib_a-ef_sqrt.o: ef_sqrt.c
+	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-ef_sqrt.o `test -f 'ef_sqrt.c' || echo '$(srcdir)/'`ef_sqrt.c
 
-lib_a-sf_sqrt.obj: sf_sqrt.c
-	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-sf_sqrt.obj `if test -f 'sf_sqrt.c'; then $(CYGPATH_W) 'sf_sqrt.c'; else $(CYGPATH_W) '$(srcdir)/sf_sqrt.c'; fi`
+lib_a-ef_sqrt.obj: ef_sqrt.c
+	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-ef_sqrt.obj `if test -f 'ef_sqrt.c'; then $(CYGPATH_W) 'ef_sqrt.c'; else $(CYGPATH_W) '$(srcdir)/ef_sqrt.c'; fi`
 
 ID: $(HEADERS) $(SOURCES) $(LISP) $(TAGS_FILES)
 	list='$(SOURCES) $(HEADERS) $(LISP) $(TAGS_FILES)'; \
diff --git a/newlib/libm/machine/riscv/s_sqrt.c b/newlib/libm/machine/riscv/e_sqrt.c
similarity index 94%
rename from newlib/libm/machine/riscv/s_sqrt.c
rename to newlib/libm/machine/riscv/e_sqrt.c
index abccf4b1c..d6bfd90ad 100644
--- a/newlib/libm/machine/riscv/s_sqrt.c
+++ b/newlib/libm/machine/riscv/e_sqrt.c
@@ -46,8 +46,6 @@ __ieee754_sqrt (double x)
 	return result;
 }
 
-#if defined(_IEEE_LIBM) && defined(HAVE_ALIAS_ATTRIBUTE)
-__strong_reference(__ieee754_sqrt, sqrt);
-#endif
-
+#else
+#include "../../math/e_sqrt.c"
 #endif
diff --git a/newlib/libm/machine/riscv/sf_sqrt.c b/newlib/libm/machine/riscv/ef_sqrt.c
similarity index 94%
rename from newlib/libm/machine/riscv/sf_sqrt.c
rename to newlib/libm/machine/riscv/ef_sqrt.c
index 9a67906c9..1f378f547 100644
--- a/newlib/libm/machine/riscv/sf_sqrt.c
+++ b/newlib/libm/machine/riscv/ef_sqrt.c
@@ -46,8 +46,6 @@ __ieee754_sqrtf (float x)
 	return result;
 }
 
-#if defined(_IEEE_LIBM) && defined(HAVE_ALIAS_ATTRIBUTE)
-__strong_reference(__ieee754_sqrtf, sqrtf);
-#endif
-
+#else
+#include "../../math/ef_sqrt.c"
 #endif
-- 
2.28.0

[PATCH 2/2] libm/riscv: Use common fma code when necessary

[Keith Packard]

For RISC-V targets without hardware FMA support, include the
common fma implementation to provide that API.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 newlib/libm/machine/riscv/s_fma.c  | 2 ++
 newlib/libm/machine/riscv/sf_fma.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/newlib/libm/machine/riscv/s_fma.c b/newlib/libm/machine/riscv/s_fma.c
index b7f378071..2d9ebfc99 100644
--- a/newlib/libm/machine/riscv/s_fma.c
+++ b/newlib/libm/machine/riscv/s_fma.c
@@ -46,4 +46,6 @@ fma (double x, double y, double z)
 	return result;
 }
 
+#else
+#include "../../common/s_fma.c"
 #endif
diff --git a/newlib/libm/machine/riscv/sf_fma.c b/newlib/libm/machine/riscv/sf_fma.c
index 8061a8abb..285c54883 100644
--- a/newlib/libm/machine/riscv/sf_fma.c
+++ b/newlib/libm/machine/riscv/sf_fma.c
@@ -46,4 +46,6 @@ fmaf (float x, float y, float z)
 	return result;
 }
 
+#else
+#include "../../common/sf_fma.c"
 #endif
-- 
2.28.0

Re: [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt

[Kito Cheng]

Hi Keith:

Thanks! and it's LGTM.

Seems like those issues can't be captured by GCC testsuite, I should
add more tests to my regular patch review test list :)

On Fri, Sep 4, 2020 at 4:55 AM Keith Packard via Newlib
<newlib@sourceware.org> wrote:
>
> RISC-V has a couple of acceleration paths that are designed to repace
> the general code for sqrt and fma.
>
> The sqrt code was using the wrong filenames and ended up replacing the
> wrong functions (sqrt instead of __ieee754_sqrt). For builds that
> disabled errno, that code provided an alias for sqrt mapping to
> __ieee754_sqrt so things "worked".
>
> The fma code had the right filenames, but because it only generated
> code on machines with hardware support, machines lacking it ended up
> replacing the useful general implementation with an empty object file.
>
> To fix this, I've renamed the RISC-V sqrt files then fixed both sqrt
> and fma to #include the general code when the machine-specific code
> wasn't used.
>
> I tested the result by making sure every libm.a generated for RISC-V
> contained exactly one of each relevant function.
>
>

Re: [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 720 bytes --]

Kito Cheng <kito.cheng@gmail.com> writes:

> Hi Keith:
>
> Thanks! and it's LGTM.
>
> Seems like those issues can't be captured by GCC testsuite, I should
> add more tests to my regular patch review test list :)

I've got a bunch of errno/exception tests that I've written for picolibc
that catch missing errno values, which is what the sqrt difference would
have caused. As for fma, you'd have to construct a test that checked for
the difference between a true fma and (a*b)+c that the general code in
newlib currently uses.

I've now added a test for picolibc to make sure there aren't duplicate
symbols in libm.a and libc.a, but that doesn't make sure you get the
*right* symbols ...

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt

[Corinna Vinschen]

On Sep  3 13:54, Keith Packard via Newlib wrote:
> RISC-V has a couple of acceleration paths that are designed to repace
> the general code for sqrt and fma.
> 
> The sqrt code was using the wrong filenames and ended up replacing the
> wrong functions (sqrt instead of __ieee754_sqrt). For builds that
> disabled errno, that code provided an alias for sqrt mapping to
> __ieee754_sqrt so things "worked".
> 
> The fma code had the right filenames, but because it only generated
> code on machines with hardware support, machines lacking it ended up
> replacing the useful general implementation with an empty object file.
> 
> To fix this, I've renamed the RISC-V sqrt files then fixed both sqrt
> and fma to #include the general code when the machine-specific code
> wasn't used.
> 
> I tested the result by making sure every libm.a generated for RISC-V
> contained exactly one of each relevant function.
> 

Pushed.

Thanks,
Corinna

Re: [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt

[Joseph Myers]

On Thu, 3 Sep 2020, Keith Packard via Newlib wrote:

> I've got a bunch of errno/exception tests that I've written for picolibc
> that catch missing errno values, which is what the sqrt difference would
> have caused. As for fma, you'd have to construct a test that checked for
> the difference between a true fma and (a*b)+c that the general code in
> newlib currently uses.

glibc has a great many tests of expected libm function return values, 
errno and exceptions, both manually maintained (libm-test-*.inc) and with 
expected results generated by MPFR (auto-libm-test-in generating 
auto-libm-test-out-* via gen-auto-libm-tests.c).  It would be interesting 
to see what those tests show up as bugs when run on different libm 
implementations (and vice versa, of course, running other libm 
implementations' tests on glibc).  However, it's probably nontrivial to 
get the tests running on other implementations, since they freely make use 
of glibc-specific features, and the particular glibc choices they test for 
regarding errno, exceptions and expected accuracy of results may not match 
the choices made by other libm implementations.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1562 bytes --]

Joseph Myers <joseph@codesourcery.com> writes:

> However, it's probably nontrivial to get the tests running on other
> implementations, since they freely make use of glibc-specific
> features, and the particular glibc choices they test for regarding
> errno, exceptions and expected accuracy of results may not match the
> choices made by other libm implementations.

I agree -- I looked at getting the glibc tests running on newlib and
decided that it would be more effort that I could afford. For now, I've
got the original newlib test suite running after spending several months
fixing it.

In particular, for the math tests, I ran glibc against the test vectors
and replaced the expected results with those as the existing values had
become corrupted over the years (not just inaccurate, but wildly
wrong). Mismatches between newlib and glibc were investigated, and
patches to newlib have been integrated. No patches were needed for
glibc. Now newlib generates results that match glibc within reasonable
tolerance bounds. With the recent patches to fix up exceptions and errno
values, newlib and glibc match there as well. I guess this is kinda like
testing newlib against the glibc test suite at a distance -- glibc gets
tested, then we make sure newlib and glibc generate the same values for
a different set of tests...

I'd love to be able to spend time improving the accuracy of newlib code,
but just having code which constantly measures 'reasonably close' to
glibc is a huge improvement from my perspective.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]