* [PATCH 1/2] libm/riscv: Fix machine-specific sqrt build process
2020-09-03 20:54 [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt Keith Packard
@ 2020-09-03 20:54 ` Keith Packard
2020-09-03 20:54 ` [PATCH 2/2] libm/riscv: Use common fma code when necessary Keith Packard
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Keith Packard @ 2020-09-03 20:54 UTC (permalink / raw)
To: newlib
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 2/2] libm/riscv: Use common fma code when necessary
2020-09-03 20:54 [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt Keith Packard
2020-09-03 20:54 ` [PATCH 1/2] libm/riscv: Fix machine-specific sqrt build process Keith Packard
@ 2020-09-03 20:54 ` Keith Packard
2020-09-04 3:07 ` [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt Kito Cheng
2020-09-04 13:11 ` Corinna Vinschen
3 siblings, 0 replies; 8+ messages in thread
From: Keith Packard @ 2020-09-03 20:54 UTC (permalink / raw)
To: newlib
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt
2020-09-03 20:54 [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt Keith Packard
2020-09-03 20:54 ` [PATCH 1/2] libm/riscv: Fix machine-specific sqrt build process Keith Packard
2020-09-03 20:54 ` [PATCH 2/2] libm/riscv: Use common fma code when necessary Keith Packard
@ 2020-09-04 3:07 ` Kito Cheng
2020-09-04 4:51 ` Keith Packard
2020-09-04 13:11 ` Corinna Vinschen
3 siblings, 1 reply; 8+ messages in thread
From: Kito Cheng @ 2020-09-04 3:07 UTC (permalink / raw)
To: Keith Packard; +Cc: Newlib
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.
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt
2020-09-04 3:07 ` [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt Kito Cheng
@ 2020-09-04 4:51 ` Keith Packard
2020-09-07 17:41 ` Joseph Myers
0 siblings, 1 reply; 8+ messages in thread
From: Keith Packard @ 2020-09-04 4:51 UTC (permalink / raw)
To: Kito Cheng; +Cc: Newlib
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt
2020-09-04 4:51 ` Keith Packard
@ 2020-09-07 17:41 ` Joseph Myers
2020-09-07 22:52 ` Keith Packard
0 siblings, 1 reply; 8+ messages in thread
From: Joseph Myers @ 2020-09-07 17:41 UTC (permalink / raw)
To: Keith Packard; +Cc: Kito Cheng, Newlib
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt
2020-09-07 17:41 ` Joseph Myers
@ 2020-09-07 22:52 ` Keith Packard
0 siblings, 0 replies; 8+ messages in thread
From: Keith Packard @ 2020-09-07 22:52 UTC (permalink / raw)
To: Joseph Myers; +Cc: Kito Cheng, Newlib
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt
2020-09-03 20:54 [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt Keith Packard
` (2 preceding siblings ...)
2020-09-04 3:07 ` [PATCH 0/2] libm/riscv: Fixing machine-specific fma/sqrt Kito Cheng
@ 2020-09-04 13:11 ` Corinna Vinschen
3 siblings, 0 replies; 8+ messages in thread
From: Corinna Vinschen @ 2020-09-04 13:11 UTC (permalink / raw)
To: newlib
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
^ permalink raw reply [flat|nested] 8+ messages in thread