Re: Subnormal cancellation correctness

[Tim Janik <timj gtk org>]

On Mon, 3 Apr 2006, Stefan Westerfeld wrote:

>   Hi!
>
> I've extended subnormals.cc to perform correctness tests as well. It
> checks both variants - float and double - offered by BSE. Here is the
> patch. Ok to commit? Or should I put the code in a seperate test?

hmmmm....
your code looks mostly good to me, a couple minor comments:
- i plan on writing a set of short standard macros for tests, to be used instead
  of your g_print/fflush/g_assert. however, i'll adapt your code accordingly
  once i have the header in a properly includable location.
- there're soem coding style issues, commented in patch.
- if subnormals.cc is really going to be used for long-standing performance
  tests, it can still be seperated out. so adding correctness tests to it now
  makes sense. thanks for tackling.

>   Cu... Stefan
>
> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/gnome/beast/bse/ChangeLog,v
> retrieving revision 1.605
> diff -u -p -r1.605 ChangeLog
> --- ChangeLog	2 Apr 2006 16:20:29 -0000	1.605
> +++ ChangeLog	3 Apr 2006 13:54:17 -0000
> @@ -1,3 +1,8 @@
> +Mon Apr  3 15:50:14 2006  Stefan Westerfeld <stefan space twc de>
> +
> +	* tests/subnormals.cc: Added code which verifies that subnormal
> +	elimination functions are correct.
> +
> Sun Apr  2 18:19:42 2006  Tim Janik  <timj gtk org>
>
> 	* bse/Makefile.am: some more dependency and minor build fixes.
> Index: tests/subnormals.cc
> ===================================================================
> RCS file: /cvs/gnome/beast/bse/tests/subnormals.cc,v
> retrieving revision 1.5
> diff -u -p -r1.5 subnormals.cc
> --- tests/subnormals.cc	1 Apr 2006 14:42:43 -0000	1.5
> +++ tests/subnormals.cc	3 Apr 2006 13:54:17 -0000
> @@ -1,5 +1,6 @@
> /* BSE - Bedevilled Sound Engine
>  * Copyright (C) 2006 Tim Janik
> + * Copyright (C) 2006 Stefan Westerfeld
>  *
>  * This library is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Lesser General Public
> @@ -18,6 +19,7 @@
>  */
> #include <bse/bse.h>
> #include <bse/bseieee754.h>
> +#include <stdio.h>
>
> #if 1
> inline float    test1 (float v) { return v;     }
> @@ -33,6 +35,53 @@ extern float    test4 (float v);
> extern float    test5 (float v);
> #endif
>
> +inline double test2d (double v) { return bse_double_zap_denormal (v); }
> +inline double test3d (double v) { BSE_DOUBLE_FLUSH_with_cond (v); return v; }
> +inline double test4d (double v) { BSE_DOUBLE_FLUSH_with_if (v); return v; }
> +inline double test5d (double v) { BSE_DOUBLE_FLUSH_with_threshold (v); return v; }

i think with these added, you should rename the above to test1f, test2f,...

> +
> +template<float Func(float)>
> +void
> +test_correct_subnormal_elimination (const char* algo_name)

there's a space missing here and an extra newline, i.e.
the function should written as:

template<float Func (float)> void
test_correct_subnormal_elimination (const char* algo_name)
{}

or

template <float Func (float)> void
test_correct_subnormal_elimination (const char* algo_name)
{}


> +{
> +  g_print ("testing algorithm %s for correctness... ", algo_name);
> +  fflush (stdout);
> +  const int n = 1000000;
> +  for (int i = 1; i < n; i++)
> +    {
> +      float value = BSE_FLOAT_MAX_SUBNORMAL * i / n;
> +      g_assert (BSE_FLOAT_IS_SUBNORMAL (value));
> +
> +      float normalized_positive_value = Func (value);
> +      g_assert (!BSE_FLOAT_IS_SUBNORMAL (normalized_positive_value));
> +
> +      float normalized_negative_value = Func (-value);
> +      g_assert (!BSE_FLOAT_IS_SUBNORMAL (normalized_negative_value));
> +    }
> +  g_print ("PASSED\n");
> +}
> +
> +template<double Func(double)>
> +void
> +test_correct_subnormal_elimination (const char* algo_name)

same prototype coding style issue.

> +{
> +  g_print ("testing algorithm %s for correctness... ", algo_name);
> +  fflush (stdout);
> +  const int n = 1000000;

hm, this is the third time we have n = 1000000 here now, right?
we should probably make that a global const then, what do you think?


> +  for (int i = 1; i < n; i++)
> +    {
> +      double value = BSE_DOUBLE_MAX_SUBNORMAL * i / n;
> +      g_assert (BSE_DOUBLE_IS_SUBNORMAL (value));
> +
> +      double normalized_positive_value = Func (value);
> +      g_assert (!BSE_DOUBLE_IS_SUBNORMAL (normalized_positive_value));
> +
> +      double normalized_negative_value = Func (-value);
> +      g_assert (!BSE_DOUBLE_IS_SUBNORMAL (normalized_negative_value));
> +    }
> +  g_print ("PASSED\n");
> +}
> +
> int
> main (int   argc,
>       char *argv[])
> @@ -122,6 +171,16 @@ main (int   argc,
>
>   g_print ("subnormal cancellation times: keep=%fs zap=%fs inlined-cond=%fs if-cond=%fs arithmetic=%f\n",
>            test1_time, test2_time, test3_time, test4_time, test5_time);
> +
> +  test_correct_subnormal_elimination<test2>("zap");
> +  test_correct_subnormal_elimination<test3>("inlined-cond");
> +  test_correct_subnormal_elimination<test4>("if-cond");
> +  test_correct_subnormal_elimination<test5>("arithmetic");

you're missing a space before '(', i.e.:
  test_correct_subnormal_elimination<test2> ("zap");

> +
> +  test_correct_subnormal_elimination<test2d>("zap-double");
> +  test_correct_subnormal_elimination<test3d>("inlined-cond-double");
> +  test_correct_subnormal_elimination<test4d>("if-cond-double");
> +  test_correct_subnormal_elimination<test5d>("arithmetic-double");
>
>   return 0;
> }
> --
> Stefan Westerfeld, Hamburg/Germany, http://space.twc.de/~stefan

---
ciaoTJ