Re: [patches] [PATCH] Make libm smaller using wrappers

[Maxim Kuvyrkov <maxim@xxxxxxxxxxxxxxxx>]

Joseph S. Myers wrote:
> On Wed, 28 Oct 2009, Maxim Kuvyrkov wrote:
>
> > @@ -645,19 +652,19 @@ sub get_ulps {
> >      # Return 0 instead of BUILD_COMPLEX (0,0)
> >      if (!exists $results{$test}{'real'}{'ulp'}{$float} &&
> >  	!exists $results{$test}{'imag'}{'ulp'}{$float}) {
> > -      return "0";
> > +      return "BUILD_COMPLEX ($default_ulp, $default_ulp)";
>
> Note the comment saying it is deliberately doing something that it is no 
> longer doing after your patch.  How is the result used, and why is this 
> change OK?  Certainly the comment will need removing or changing if the 
> output change stays.

This clause outputs default error with which results of complex 
functions are compared against expected [complex] values.  If I 
understand GNU C __complex__ correctly, there's no difference between 
'0' and '0 + 0i'; if so, then using "0" instead of "BUILD_COMPLEX (0,0)" 
in the above clause is just an optimization.  The result of the above 
clause is passed to a function that expects '__complex__ <float type>' 
parameter, so the optimization appears to be quite minor in practice.

> All new tests should be using the test-skeleton infrastructure if 
> possible.

Noted.  I'll update the testcase.

> Otherwise the patch seems OK, though it would be good to have specific 
> guidance somewhere (a mailing list message would be OK) on how to 
> determine what changes are needed to keep this option group working 
> correctly if a new libm function is added.

That's good suggestion!  See below.

> Do I understand correctly that 
> no change is strictly needed for any given function - but if no change is 
> made all three versions of the new function will be present whatever the 
> setting for this option group?

Yes, that's correct given that implementation of the new function does 
not directly include any of the .c files.  Implementations of the new 
functions have little value in including other .c files, so special care 
is need only when dealing with compatibility code like in ldbl-opt/.

If the implementation does include .c files, then it will need changes 
to include the correct file which depends on the option group selection. 
 Generally, something like this
<cut>
--- a/sysdeps/ieee754/ldbl-opt/s_atan.c
+++ b/sysdeps/ieee754/ldbl-opt/s_atan.c
@@ -1,5 +1,10 @@
 #include <math_ldbl_opt.h>
-#include <sysdeps/ieee754/dbl-64/s_atan.c>
+#include <gnu/option-groups.h>
+#if __OPTION_EGLIBC_LIBM_BIG
+# include <sysdeps/ieee754/dbl-64/s_atan.c>
+#else
+# include <sysdeps/ieee754/dbl-wrap/s_atan.c>
+#endif
 #if LONG_DOUBLE_COMPAT(libm, GLIBC_2_0)
 compat_symbol (libm, atan, atanl, GLIBC_2_0);
 #endif
</cut>
will be enough.

> If so, the documentation would be saying 
> how to make the option group effective for a new function that is making a 
> significant contribution to libm size.  (It might of course also be useful 
> for any new functions to get an associated option group to disable a 
> family of functions.)

The process of adding handling of a new function to wrappers has the 
following steps:

0. We have a new function 'ret_type __fnl (arg_type arg)' with 
implemention in, say, ldbl-xxx/fnl.c.  This function has an alias 'long 
double fnl (long double arg)'.

1. Create file ldbl-wrap/fnl-wrap.c following the template:
<cut>
#include "ldbl-wrap.h"

/* This expands into 'float __fnf (float)'.  */
wrap_type_t WRAP_FUNC (__fn) (wrap_type_t);

long double
__fnl (long double x)
{
  /* This expands into
     return (long double) __fnf ((float) x);  */
  return (long double) WRAP_FUNC (__fn) ((wrap_type_t) x);
}
</cut>

2. Use "#if __OPTION_EGLIBC_LIBM_BIG" to disable original code in 
ldbl-xxx/fnl.c.

+#include <gnu/option-groups.h>
+
+#if __OPTION_EGLIBC_LIBM_BIG

/* Big chunk of code */

+#else /* !__OPTION_EGLIBC_LIBM_BIG */

3. In the #else branch include ldbl-wrap/fnl-wrap.c.

+# include <sysdeps/ieee754/ldbl-wrap/fnl-wrap.c>

4. With the big chunk of code you've probably also disabled creation of 
alias from __fnl() to fnl(); add a copy of alias after the #include. 
Don't forget to close the #if statement.

+weak_alias (__erfl, erfl)
+#endif /* __OPTION_EGLIBC_LIBM_BIG */

The work I've done follows the above recipe for ldbl-* functions.  The 
wrappers for dbl-64/ can be optimized a bit due to the fact that there's 
only one dbl implementation, so alias handling does not require special 
attention.  One can use dbl-wrap/fn.c instead of dbl-wrap/fn-wrap.c and 
have the alias directives right there.

--
Maxim Kuvyrkov
CodeSourcery
maxim@xxxxxxxxxxxxxxxx
(650) 331-3385 x724