libm -fno-builtin

Hi,

I've tried removing -fno-builtin for libm and the results look good. As well
as fabs and floor, gcc also inlines creal, cimag, carg, copysign and finite.
Some of the complex maths functions are greatly simplified, as creal & cimag
can effectively be optimised away, whereas at the moment the function calls
result in lots of stack manipulation. I can't see anything obviously
problematic with this.

Overall, for my embedded target, libm code size reduced by about 4%.

Cheers,
Jon

Here's a few places where it can be removed - there's also other uses in
machine/*/configure - but I guess that is best left to the maintainers of
those ports to remove.

diff --git a/newlib/libm/configure b/newlib/libm/configure
index 961f0cb6d..f37fae2a7 100755
--- a/newlib/libm/configure
+++ b/newlib/libm/configure
@@ -3725,7 +3725,7 @@ fi

. ${newlib_basedir}/configure.host

-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"

NEWLIB_CFLAGS=${newlib_cflags}

diff --git a/newlib/libm/machine/configure b/newlib/libm/machine/configure
index c15ef32ee..2b9c5c1b9 100755
--- a/newlib/libm/machine/configure
+++ b/newlib/libm/machine/configure
@@ -3671,7 +3671,7 @@ fi

. ${newlib_basedir}/configure.host

-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"

NEWLIB_CFLAGS=${newlib_cflags}

diff --git a/newlib/libm/machine/i386/configure
b/newlib/libm/machine/i386/configure
index f6bf9892a..9f07b7bd2 100755
--- a/newlib/libm/machine/i386/configure
+++ b/newlib/libm/machine/i386/configure
@@ -3661,7 +3661,7 @@ fi

. ${newlib_basedir}/configure.host

-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"

NEWLIB_CFLAGS=${newlib_cflags}

Corinna Vinschen

2018-06-21 07:30:37 UTC

Post by Jon Beniston
Hi,
I've tried removing -fno-builtin for libm and the results look good. As well
as fabs and floor, gcc also inlines creal, cimag, carg, copysign and finite.
Some of the complex maths functions are greatly simplified, as creal & cimag
can effectively be optimised away, whereas at the moment the function calls
result in lots of stack manipulation. I can't see anything obviously
problematic with this.
Overall, for my embedded target, libm code size reduced by about 4%.
Cheers,
Jon
Here's a few places where it can be removed - there's also other uses in
machine/*/configure - but I guess that is best left to the maintainers of
those ports to remove.

Care to provide a git format-patch created patch, please?

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Jon Beniston

2018-06-22 09:43:02 UTC

Post by Corinna Vinschen
Care to provide a git format-patch created patch, please?

Here's an updated patch to remove -fno-builtin for all targets, hopefully in the correct format.

Cheers,
Jon

From: Jon Beniston <***@beniston.com>
Date: Fri, 22 Jun 2018 10:39:39 +0100
Subject: [PATCH 1/1] Remove -fno-builtin to allow gcc to inline functions such
as fabs, floor, creal, imag.

---
newlib/libm/configure | 2 +-
newlib/libm/machine/aarch64/configure | 2 +-
newlib/libm/machine/arm/configure | 2 +-
newlib/libm/machine/configure | 2 +-
newlib/libm/machine/i386/configure | 2 +-
newlib/libm/machine/nds32/configure | 2 +-
newlib/libm/machine/riscv/configure | 2 +-
newlib/libm/machine/spu/configure | 2 +-
8 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/newlib/libm/configure b/newlib/libm/configure
index 42d503640..657ca3c24 100755
--- a/newlib/libm/configure
+++ b/newlib/libm/configure
@@ -3725,7 +3725,7 @@ fi

. ${newlib_basedir}/configure.host

-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"

NEWLIB_CFLAGS=${newlib_cflags}

diff --git a/newlib/libm/machine/aarch64/configure b/newlib/libm/machine/aarch64/configure
index 397abd875..1ce90bcd2 100755
--- a/newlib/libm/machine/aarch64/configure
+++ b/newlib/libm/machine/aarch64/configure
@@ -3335,7 +3335,7 @@ fi

. ${newlib_basedir}/configure.host

-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"

NEWLIB_CFLAGS=${newlib_cflags}

diff --git a/newlib/libm/machine/arm/configure b/newlib/libm/machine/arm/configure
index 397abd875..1ce90bcd2 100755
--- a/newlib/libm/machine/arm/configure
+++ b/newlib/libm/machine/arm/configure
@@ -3335,7 +3335,7 @@ fi

. ${newlib_basedir}/configure.host

-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"

NEWLIB_CFLAGS=${newlib_cflags}

diff --git a/newlib/libm/machine/configure b/newlib/libm/machine/configure
index 2c237a126..a8255e0d4 100755
--- a/newlib/libm/machine/configure
+++ b/newlib/libm/machine/configure
@@ -3671,7 +3671,7 @@ fi

. ${newlib_basedir}/configure.host

-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"

NEWLIB_CFLAGS=${newlib_cflags}

diff --git a/newlib/libm/machine/i386/configure b/newlib/libm/machine/i386/configure
index 0d5fae926..7fea03b2f 100755
--- a/newlib/libm/machine/i386/configure
+++ b/newlib/libm/machine/i386/configure
@@ -3661,7 +3661,7 @@ fi

. ${newlib_basedir}/configure.host

-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"

NEWLIB_CFLAGS=${newlib_cflags}

diff --git a/newlib/libm/machine/nds32/configure b/newlib/libm/machine/nds32/configure
index 43ea30a59..04214e714 100644
--- a/newlib/libm/machine/nds32/configure
+++ b/newlib/libm/machine/nds32/configure
@@ -3358,7 +3358,7 @@ fi

. ${newlib_basedir}/configure.host

-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"

NEWLIB_CFLAGS=${newlib_cflags}

diff --git a/newlib/libm/machine/riscv/configure b/newlib/libm/machine/riscv/configure
index 397abd875..1ce90bcd2 100755
--- a/newlib/libm/machine/riscv/configure
+++ b/newlib/libm/machine/riscv/configure
@@ -3335,7 +3335,7 @@ fi

. ${newlib_basedir}/configure.host

-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"

NEWLIB_CFLAGS=${newlib_cflags}

diff --git a/newlib/libm/machine/spu/configure b/newlib/libm/machine/spu/configure
index 397abd875..1ce90bcd2 100644
--- a/newlib/libm/machine/spu/configure
+++ b/newlib/libm/machine/spu/configure
@@ -3335,7 +3335,7 @@ fi

. ${newlib_basedir}/configure.host

-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"

NEWLIB_CFLAGS=${newlib_cflags}

--
2.15.1

Corinna Vinschen

2018-06-25 11:31:09 UTC

Post by Corinna Vinschen
Care to provide a git format-patch created patch, please?

Here's an updated patch to remove -fno-builtin for all targets,
hopefully in the correct format.

Well, not quite. Either just send the below mail file with
`git send-email', or attach the file. Both methods allow the
maintainers to just use the mail or the attachment as is, rather
than having to edit it before applying.

Thanks,
Corinna

Post by Jon Beniston
Cheers,
Jon
Date: Fri, 22 Jun 2018 10:39:39 +0100
Subject: [PATCH 1/1] Remove -fno-builtin to allow gcc to inline functions such
as fabs, floor, creal, imag.
---
newlib/libm/configure | 2 +-
newlib/libm/machine/aarch64/configure | 2 +-
newlib/libm/machine/arm/configure | 2 +-
newlib/libm/machine/configure | 2 +-
newlib/libm/machine/i386/configure | 2 +-
newlib/libm/machine/nds32/configure | 2 +-
newlib/libm/machine/riscv/configure | 2 +-
newlib/libm/machine/spu/configure | 2 +-
8 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/newlib/libm/configure b/newlib/libm/configure
index 42d503640..657ca3c24 100755
--- a/newlib/libm/configure
+++ b/newlib/libm/configure
@@ -3725,7 +3725,7 @@ fi
. ${newlib_basedir}/configure.host
-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"
NEWLIB_CFLAGS=${newlib_cflags}
diff --git a/newlib/libm/machine/aarch64/configure b/newlib/libm/machine/aarch64/configure
index 397abd875..1ce90bcd2 100755
--- a/newlib/libm/machine/aarch64/configure
+++ b/newlib/libm/machine/aarch64/configure
@@ -3335,7 +3335,7 @@ fi
. ${newlib_basedir}/configure.host
-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"
NEWLIB_CFLAGS=${newlib_cflags}
diff --git a/newlib/libm/machine/arm/configure b/newlib/libm/machine/arm/configure
index 397abd875..1ce90bcd2 100755
--- a/newlib/libm/machine/arm/configure
+++ b/newlib/libm/machine/arm/configure
@@ -3335,7 +3335,7 @@ fi
. ${newlib_basedir}/configure.host
-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"
NEWLIB_CFLAGS=${newlib_cflags}
diff --git a/newlib/libm/machine/configure b/newlib/libm/machine/configure
index 2c237a126..a8255e0d4 100755
--- a/newlib/libm/machine/configure
+++ b/newlib/libm/machine/configure
@@ -3671,7 +3671,7 @@ fi
. ${newlib_basedir}/configure.host
-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"
NEWLIB_CFLAGS=${newlib_cflags}
diff --git a/newlib/libm/machine/i386/configure b/newlib/libm/machine/i386/configure
index 0d5fae926..7fea03b2f 100755
--- a/newlib/libm/machine/i386/configure
+++ b/newlib/libm/machine/i386/configure
@@ -3661,7 +3661,7 @@ fi
. ${newlib_basedir}/configure.host
-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"
NEWLIB_CFLAGS=${newlib_cflags}
diff --git a/newlib/libm/machine/nds32/configure b/newlib/libm/machine/nds32/configure
index 43ea30a59..04214e714 100644
--- a/newlib/libm/machine/nds32/configure
+++ b/newlib/libm/machine/nds32/configure
@@ -3358,7 +3358,7 @@ fi
. ${newlib_basedir}/configure.host
-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"
NEWLIB_CFLAGS=${newlib_cflags}
diff --git a/newlib/libm/machine/riscv/configure b/newlib/libm/machine/riscv/configure
index 397abd875..1ce90bcd2 100755
--- a/newlib/libm/machine/riscv/configure
+++ b/newlib/libm/machine/riscv/configure
@@ -3335,7 +3335,7 @@ fi
. ${newlib_basedir}/configure.host
-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"
NEWLIB_CFLAGS=${newlib_cflags}
diff --git a/newlib/libm/machine/spu/configure b/newlib/libm/machine/spu/configure
index 397abd875..1ce90bcd2 100644
--- a/newlib/libm/machine/spu/configure
+++ b/newlib/libm/machine/spu/configure
@@ -3335,7 +3335,7 @@ fi
. ${newlib_basedir}/configure.host
-newlib_cflags="${newlib_cflags} -fno-builtin"
+newlib_cflags="${newlib_cflags}"
NEWLIB_CFLAGS=${newlib_cflags}
--
2.15.1

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Corinna Vinschen

2018-06-25 11:37:24 UTC

Post by Corinna Vinschen
Care to provide a git format-patch created patch, please?

Here's an updated patch to remove -fno-builtin for all targets,
hopefully in the correct format.

Uhm... even more importantly, your patch is changing the configure files
directly, rather than changing the configure.in files the configure
files are generated from. Your below patch would be overwritten next
time the configure files are regenerated from their configure.in source
files.

For the patch, just change the configure.in files and send the below
manual configure changes alongside. The patch is easy enough to go
without actually jumping throught the autoconf hoop.

Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Jon Beniston

2018-06-25 12:47:33 UTC

Hi Corinna,

Uhm... even more importantly, your patch is changing the configure files directly,
rather than changing the configure.in files the configure files are generated from.
For the patch, just change the configure.in files and send the below manual configure changes alongside

-fno-builtin isn't in configure.in. Where does that come from?

Cheers,
Jon

Jon Beniston

2018-06-25 13:26:18 UTC

-fno-builtin isn't in configure.in. Where does that come from?

It looks like it comes from a common acinclude.m4 that is shared with libc, where I presume -fno-builtin should still be used. Any ideas how that should be resolved?

Cheers,
Jon

Corinna Vinschen

2018-06-25 13:27:17 UTC

Post by Jon Beniston
Hi Corinna,

-fno-builtin isn't in configure.in. Where does that come from?

Huh, I didn't check before writing my reply, but in fact it's in
acinclude.m4. The line is unchanged since the dawn of time,
AKA creation of the original sourceware CVS repo back in 2000.

Jeff, any idea why it's there and if this is more than just a
remnant of the dark ages?

Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Jeff Johnston

2018-06-25 15:44:11 UTC

Post by Jon Beniston
Hi Corinna,

Post by Corinna Vinschen
Uhm... even more importantly, your patch is changing the configure

files directly,

Post by Corinna Vinschen
rather than changing the configure.in files the configure files are

generated from.

Post by Corinna Vinschen
For the patch, just change the configure.in files and send the below

manual configure changes alongside

Post by Jon Beniston
-fno-builtin isn't in configure.in. Where does that come from?

Huh, I didn't check before writing my reply, but in fact it's in
acinclude.m4. The line is unchanged since the dawn of time,
AKA creation of the original sourceware CVS repo back in 2000.
Jeff, any idea why it's there and if this is more than just a
remnant of the dark ages?

I don't know the history of it.

Corinna
--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Corinna Vinschen

2018-06-26 08:22:39 UTC

Post by Jon Beniston
Hi Corinna,

Post by Corinna Vinschen
Uhm... even more importantly, your patch is changing the configure

files directly,

Post by Corinna Vinschen
rather than changing the configure.in files the configure files are

generated from.

Post by Corinna Vinschen
For the patch, just change the configure.in files and send the below

manual configure changes alongside

Post by Jon Beniston
-fno-builtin isn't in configure.in. Where does that come from?

Huh, I didn't check before writing my reply, but in fact it's in
acinclude.m4. The line is unchanged since the dawn of time,
AKA creation of the original sourceware CVS repo back in 2000.
Jeff, any idea why it's there and if this is more than just a
remnant of the dark ages?

I don't know the history of it.

That's a bit disappointing. However, just as Jon it's not clear to
me how to proceed. ANy input?

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Jeff Johnston

2018-06-26 17:08:49 UTC

I suggest it still be defaulted but a configuration switch added.
Platforms can override in configure.host to make their own
default or users can experiment with the option turned off/on if desired.
What builtins are provided for each platform and how well they work and how
they will affect the
build is unknown at this point.

-- Jeff J.

Post by Jon Beniston
Hi Corinna,

Post by Corinna Vinschen
Uhm... even more importantly, your patch is changing the configure

files directly,

Post by Corinna Vinschen
rather than changing the configure.in files the configure files

are

generated from.

Post by Corinna Vinschen
For the patch, just change the configure.in files and send the

below

manual configure changes alongside

Post by Jon Beniston
-fno-builtin isn't in configure.in. Where does that come from?

Huh, I didn't check before writing my reply, but in fact it's in
acinclude.m4. The line is unchanged since the dawn of time,
AKA creation of the original sourceware CVS repo back in 2000.
Jeff, any idea why it's there and if this is more than just a
remnant of the dark ages?

I don't know the history of it.

That's a bit disappointing. However, just as Jon it's not clear to
me how to proceed. ANy input?
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
Red Hat

j***@beniston.com

2018-08-16 22:10:58 UTC

Post by Jeff Johnston
I suggest it still be defaulted but a configuration switch added.
Platforms can override in configure.host to make their own default or users can experiment with the option turned off/on if desired.
What builtins are provided for each platform and how well they work and how they will affect the build is unknown at this point.

I tried building libc without -fno-builtin as well, and for my target at least, there weren't too many changes. A couple of things I noticed were:

- strlen etc inlined.
- bzero replaced by call to memset.
- puts changed to fwrite

Which all seem ok. So attached is a patch to the configure scripts, which I hope is roughly what you have suggested. I've added the configure option --disable-newlib-fno-builtin which prevents -fno-builtin from being used, and it should default to being enabled.

Cheers,
Jon

j***@beniston.com

2018-08-29 16:05:32 UTC

Post by j***@beniston.com
So attached is a patch to the configure scripts, which I hope is
roughly what you have suggested. I've added the configure option
--disable-newlib-fno-builtin which prevents -fno-builtin from being used,
and it should default to being enabled.

Any thoughts on this patch Corinna or Jeff?

Thanks,
Jon

Corinna Vinschen

2018-08-29 16:10:26 UTC

Post by j***@beniston.com

Any thoughts on this patch Corinna or Jeff?

I'd like to defer to Jeff for this stuff.

Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Jeff Johnston

2018-08-29 16:58:26 UTC

I am fine with the patch. It allows users to disable the option manually
via configuration and to default as before.

The patch is missing changes to sys/linux subdirectory configure scripts if
it is to be taken as-is.

-- Jeff J.

Post by j***@beniston.com

used,

Post by j***@beniston.com

Post by j***@beniston.com
and it should default to being enabled.

Any thoughts on this patch Corinna or Jeff?

I'd like to defer to Jeff for this stuff.
Corinna
--
Corinna Vinschen
Cygwin Maintainer
Red Hat

j***@beniston.com

2018-08-31 16:48:07 UTC

I am fine with the patch. It allows users to disable the option manually via configuration and to default as before.
The patch is missing changes to sys/linux subdirectory configure scripts if it is to be taken as-is.

Thanks, didn't notice those. Updated patch attached to include those directories.

Cheers,
Jon

Freddie Chopin

2018-08-31 17:48:30 UTC

Hello Jon!

So the final opinion is that it is safe to use the new `--disable-
newlib-fno-builtin` option with ARM MCU targets, right? (;

Regards,
FCh

j***@beniston.com

2018-08-31 22:34:23 UTC

Hi Freddie,

So the final opinion is that it is safe to use the new `--disable- newlib-fno-builtin` option with ARM MCU targets, right? (;

I haven't tested it on ARM, so can't say for certain. If there is a problem, I'll look in to trying to fix it.

Regards,
FCh

Jeff Johnston

2018-08-31 19:44:53 UTC

Patch committed. Thanks.

-- Jeff J.

Post by Jeff Johnston
I am fine with the patch. It allows users to disable the option

manually via configuration and to default as before.

Post by Jeff Johnston
The patch is missing changes to sys/linux subdirectory configure scripts

if it is to be taken as-is.
Thanks, didn't notice those. Updated patch attached to include those directories.
Cheers,
Jon

Freddie Chopin

2018-06-21 08:18:04 UTC

Post by Jon Beniston
Here's a few places where it can be removed - there's also other uses in
machine/*/configure - but I guess that is best left to the
maintainers of
those ports to remove.

If you'd ask me, I'd remove them all (; It will be much easier to win
the attention of the maintainers when something breaks (very unlikely)
instead of other way aroun ("if it ain’t broke, don’t fix it").

Anyway - is there any automatic test that can be done for removal of
this option, or maybe you just "manually" (or "visually") inspected
generated assembly for some typical cases? ARM also has this option, so
I think it would be nice to remove it.

Regards,
FCh

Wilco Dijkstra

2018-06-21 13:25:18 UTC

Post by Freddie Chopin
If you'd ask me, I'd remove them all (; It will be much easier to win
the attention of the maintainers when something breaks (very unlikely)
instead of other way aroun ("if it ain’t broke, don’t fix it").

Agreed - there aren't many cases where you actually need -fno-builtin.
For math functions an obvious candidate is fabs where a compiler could
recognize common patterns. If a target doesn't have an fabs instruction (!),
you might get a call to fabs and thus an infinite loop. I don't think this could
happen with the newlib math/fabs.c.

Post by Freddie Chopin
Anyway - is there any automatic test that can be done for removal of
this option, or maybe you just "manually" (or "visually") inspected
generated assembly for some typical cases? ARM also has this option, so
I think it would be nice to remove it.

I'd build it both ways, and check number of calls has reduced to confirm inlining
of builtins. Also check for single-instruction functions tailcalling themselves.

Note -fno-math-errno is also a good option to add for libm since it enables
more inlining. I added it to GLIBC a while back, no issues found since.

Wilco

Jon Beniston

2018-06-21 16:42:40 UTC

Hi Wilco,

Post by Wilco Dijkstra
Note -fno-math-errno is also a good option to add for libm since it enables
more inlining. I added it to GLIBC a while back, no issues found since.

This causes a problem for sqrtl when _LDBL_EQ_DBL is defined. The call to
sqrt is replaced with a sqrt instruction, so errno will not be set.

I can't see many improvements in the code otherwise (there's a few changes
in scheduling) - but perhaps that is target specific.

Cheers,
Jon

Wilco Dijkstra

2018-06-21 21:56:32 UTC

Post by Jon Beniston
This causes a problem for sqrtl when _LDBL_EQ_DBL is defined. The call to
sqrt is replaced with a sqrt instruction, so errno will not be set.

Yes that's another example which needs to use -fno-builtin(-sqrt) in the makefile.

Post by Jon Beniston
I can't see many improvements in the code otherwise (there's a few changes
in scheduling) - but perhaps that is target specific.

It enables inlining of functions like lround, lrint and sqrt, and yes it only helps targets
which have inlines for those. Compilers can also do more optimization since math
functions become pure if errno is no longer assumed to be live after each call.
I've got a patch in review to change the default behaviour of GCC, so fixing any failing
cases now is good future proofing.

Wilco

Jon Beniston

2018-06-22 08:56:24 UTC

Hi Wilco,

Post by Wilco Dijkstra

Post by Jon Beniston
This causes a problem for sqrtl when _LDBL_EQ_DBL is defined. The call to
sqrt is replaced with a sqrt instruction, so errno will not be set.

Yes that's another example which needs to use -fno-builtin(-sqrt) in the makefile.

Post by Jon Beniston
I can't see many improvements in the code otherwise (there's a few changes
in scheduling) - but perhaps that is target specific.

It enables inlining of functions like lround, lrint and sqrt, and yes it only helps targets
which have inlines for those.

After a quick look, I can't see any places where libm calls lround or lrint,
other than for the long double implementations, so not sure it is beneficial
in those cases. However, libm doesn't set errno in these functions anyway
(perhaps it should though).

The code in libm/math calls __ieee754_sqrt rather than sqrt anyway. The only
place it looks like sqrt is used is in csqrt.c, where the use of fabs on the
argument should mean there aren't errors anyway.

But it seems it would affect quite a few other functions too, that are
occasionally used. Also, how do you specify per .c file options in an
Makefile.am. Seems tricky, for not much gain.

Cheers,
Jon

Wilco Dijkstra

2018-06-22 14:12:38 UTC

Jon Beniston wrote:

Post by Jon Beniston
After a quick look, I can't see any places where libm calls lround or lrint,
other than for the long double implementations, so not sure it is beneficial
in those cases. However, libm doesn't set errno in these functions anyway
(perhaps it should though).

It should since in newlib (math_errhandling & MATH_ERRNO) != 0.
lround is used by most of the new math functions in libm/common, hence the
use of -fno-math-errno.

Post by Jon Beniston
The code in libm/math calls __ieee754_sqrt rather than sqrt anyway. The only
place it looks like sqrt is used is in csqrt.c, where the use of fabs on the
argument should mean there aren't errors anyway.

It should use sqrt really and -fno-math-errno so that the compiler will inline
sqrt as a single instruction on most targets. I did this in GLIBC a while back.

Wilco

Jon Beniston

2018-06-22 15:27:36 UTC

Hi Wilco,

Post by Wilco Dijkstra
lround is used by most of the new math functions in libm/common, hence the
use of -fno-math-errno.

Ah, didn't see them, as I wasn't looking at head.

Cheers,
Jon

Richard Earnshaw (lists)

2018-06-25 16:53:26 UTC

Post by Jon Beniston
Hi,
libm is currently built with -fno-builtin. This results in sub-optimal code
for uses of functions such as fabs & floor. Without -fno-builtin, gcc can
inline these to single instructions on many targets. As is, we end up with
function calls, to slower generic code.
Can -fno-builtin therefore be removed? If there are specific builtins that
need to be prevented from being used, these can prevented with
-fno-builtin-function, perhaps even on a per file basis rather than
globally.
Are there any known traps before I give it a try?
Cheers,
Jon

I think use of -fno-builtin when building the C libraries comes from
paranoia as much as anything, but the general problem is that unless you
know exactly how every version of every C compiler will recognize
particular idioms and handle those cases explicitly, you'll occasionally
end up with cyclic definitions.

The canonical example is something like memset. memset idioms are
generally quite easy for the compiler to recognize and I've frequently
seen bug reports from naive users who write

void *
memset (void *p, size_t l)
{
....
}

and then end up with

memset:
jmp memset

because the compiler knows that the body of the function is just, well,
memset!

It gets harder when one function can be implemented in terms of another
with a different name; now you have to know which idioms the compiler
might try to rewrite.

R.

Joseph Myers

2018-06-26 19:39:24 UTC

Post by Richard Earnshaw (lists)
The canonical example is something like memset. memset idioms are
generally quite easy for the compiler to recognize and I've frequently

But -fno-builtin isn't what you want there (it's about recognising the
semantics of a call to a standard function, as opposed to generating a
call to a function not called in the source, and GCC always requires libc
to provide memcpy, memmove, memset and memcmp); to avoid converting loops
to mem* calls, you need -fno-tree-loop-distribute-patterns.

--
Joseph S. Myers
***@codesourcery.com

Wilco Dijkstra

2018-08-17 13:49:24 UTC

Hi,

Yes -fno-builtin should not be used indeed as it disables lots of useful optimizations.
Removing -fno-builtin should have no negative effects except for eg. memcpy where
compilers can recognize an overly simplistic implementation and optimize it to a
call to memcpy... I think you will need a -fno-builtin-memcpy when building the size
optimized version of libc/string/memcpy.c given it is marked with restrict.

I think a config option is overkill.

Wilco

j***@beniston.com

2018-08-17 19:20:53 UTC

Hi Wilco,

Post by Wilco Dijkstra
Removing -fno-builtin should have no negative effects except for eg. memcpy

where compilers

Post by Wilco Dijkstra
can recognize an overly simplistic implementation and optimize it to a

call to memcpy...

Post by Wilco Dijkstra
I think you will need a -fno-builtin-memcpy when building the size

optimized

Post by Wilco Dijkstra
version of libc/string/memcpy.c given it is marked with restrict.

Using -fno-builtin-memcpy doesn't prevent GCC from replacing a loop with a
call to memcpy (likewise for memset). As Joesph said before,
-fno-tree-loop-distribute-patterns should be used for that. However, this
probably it isn't needed, as this only gets enabled at -O3 anyway, and
Newlib compiles with O2.

Note that -fno-builtin does prevent this, oddly, but obviously we don't want
to use that.

Perhaps GCC should be a little bit more clever, and not try to use this
optimisation if the function called matches the name of the function being
compiled.

Cheers,
Jon

j***@beniston.com

2018-08-17 19:31:14 UTC

Post by j***@beniston.com
Using -fno-builtin-memcpy doesn't prevent GCC from replacing a loop
with a call to memcpy (likewise for memset). As Joesph said before,
-fno-tree-loop-distribute-patterns should be used for that. However,
this probably it isn't needed, as this only gets enabled at -O3 anyway,
and Newlib compiles with O2.
Note that -fno-builtin does prevent this, oddly, but obviously
we don't want to use that.
Perhaps GCC should be a little bit more clever, and not try to use this
optimisation if the function called matches the name of the function being

compiled.

Ah, there's already a PR for that:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56888

Cheers,
Jon

Wilco Dijkstra

2018-08-24 14:52:15 UTC

That looks like a bug indeed since nobody will remember to use
-fno-tree-loop-distribute-patterns.

However it is quite feasible to use -fno-builtin as an extra option on a few
files if they need it. Newlib might not need it today, but GLIBC has a few uses.

Post by j***@beniston.com
Perhaps GCC should be a little bit more clever, and not try to use this
optimisation if the function called matches the name of the function being

compiled.

That would be even better. However that doesn't work in all cases, for
example GLIBC often uses different names to avoid namespace issues.

Wilco

Keith Packard

2018-08-27 06:55:31 UTC

This post might be inappropriate. Click to display it.

Wilco Dijkstra

2018-09-01 09:52:46 UTC