Re: [PATCH] Inline C99 math functions

[OndÅej BÃlka <neleai at seznam dot cz>]

On Tue, Jun 16, 2015 at 06:53:39PM +0100, Wilco Dijkstra wrote:
> > OndÅej BÃlka wrote:
> > On Tue, Jun 16, 2015 at 04:53:11PM +0100, Wilco Dijkstra wrote:
> > > I added a new math-inlines benchmark based on the string benchmark infrastructure.
> > > I used 2x1024 inputs, one 99% finite FP numbers (20% zeroes) and 1% inf/NaN,
> > > and the 2nd with 50% inf, and 50% Nan. Here are the relative timings for Cortex-A57:
> > >
> > Where is benchmark, there are several things that could go wrong with it.
> 
> I'll send it when I can (it has to go through review etc).
> 
> > > __fpclassify_t:	8.76	7.04
> > > fpclassify_t:	4.91	5.17
> > 
> > > __isnormal_inl_t:	8.77	7.16
> > > isnormal_t:		3.16	3.17
> > 
> > Where did you get inline? I couldn't find it anywhere. Also such big
> > number for inline implementation is suspect
> 
> It does (__fpclassify (x) == FP_NORMAL) like math.h which is obviously a bad
> idea and the reason for the low performance. Although the GCC isnormal builtin
> is not particularly fast, it still beats it by more than a factor of 2.
>
No, bad idea was not inlining fpclassify, that affects most of performance difference. 
There is also problem that glibcdev/glibc/sysdeps/ieee754/dbl-64/s_fpclassify.c is bit slow as it tests unlikely cases first but that is secondary.

> > > __finite_inl_t:	1.91	1.91
> > > __finite_t:		15.29	15.28
> > > isfinite_t:		1.28	1.28
> > > __isinf_inl_t:	1.92	2.99
> > > __isinf_t:		8.9	6.17
> > > isinf_t:		1.28	1.28
> > > __isnan_inl_t:	1.91	1.92
> > > __isnan_t:		15.28	15.28
> > > isnan_t:		1	1.01
> > >
> > > The plain isnan_t functions use the GCC built-ins, the _inl variant uses the
> > > existing math_private.h inlines (with __isinf fixed to return the sign too),
> > > and the __isnan variants are the non-inline GLIBC functions.
> > >
> > > So this clearly shows the GCC built-ins win by a huge margin, including the
> > > inline versions.
> > That looks bit suspect, submit a benchmark to see if its correct or not.
> 
> It's certainly correct, but obviously different microarchitectures will show
> different results. Note the GLIBC private inlines are not particularly good.
>
No, problem is that different benchmarks show different results on same
architecture. To speed things up run following to test all cases of
environment. Run attached tf script to get results on arm.

I am concerned on libc inlines that they need two big constants. That
sohuldn't be problem with loop but should in initial checks, so I need
benchmark to see what case you did test. Alter that only thing I could
come is that move from fp register has slow latency compared to floating
comparison. That don't have to mean anything on OoO cpu which hide that
latency by executing instructions after that. Second theory is that gcc
decided that its infinity is likely which leads to suboptimal assembly. 
We should also add expect to header to add that information


On x64 I get following results depending how gcc optimizes code which is
controled by inlining

don't inline
conditional add
branched

real	0m1.313s
user	0m1.312s
sys	0m0.000s
builtin

real	0m1.309s
user	0m1.308s
sys	0m0.000s
branch
branched

real	0m1.310s
user	0m1.308s
sys	0m0.000s
builtin

real	0m1.337s
user	0m1.312s
sys	0m0.004s
sum
branched

real	0m1.209s
user	0m1.204s
sys	0m0.000s
builtin

real	0m1.216s
user	0m1.212s
sys	0m0.000s
inline outer call
conditional add
branched

real	0m0.705s
user	0m0.704s
sys	0m0.000s
builtin

real	0m0.916s
user	0m0.916s
sys	0m0.000s
branch
branched

real	0m0.806s
user	0m0.804s
sys	0m0.000s
builtin

real	0m0.721s
user	0m0.716s
sys	0m0.000s
sum
branched

real	0m1.029s
user	0m1.028s
sys	0m0.000s
builtin

real	0m0.911s
user	0m0.908s
sys	0m0.000s
inline inner call
conditional add
branched

real	0m1.038s
user	0m1.032s
sys	0m0.000s
builtin

real	0m1.024s
user	0m1.016s
sys	0m0.000s
branch
branched

real	0m0.614s
user	0m0.608s
sys	0m0.000s
builtin

real	0m0.606s
user	0m0.608s
sys	0m0.000s
sum
branched

real	0m0.662s
user	0m0.660s
sys	0m0.000s
builtin

real	0m0.629s
user	0m0.628s
sys	0m0.000s
tigth loop
conditional add
branched

real	0m0.208s
user	0m0.208s
sys	0m0.000s
builtin

real	0m0.326s
user	0m0.324s
sys	0m0.000s
branch
branched

real	0m0.204s
user	0m0.200s
sys	0m0.000s
builtin

real	0m0.325s
user	0m0.324s
sys	0m0.000s
sum
branched

real	0m0.328s
user	0m0.332s
sys	0m0.000s
builtin

real	0m0.486s
user	0m0.484s
sys	0m0.000s

 
> > >  It also shows that multiple isinf/isnan calls would be faster
> > > than a single inlined fpclassify...
> > >
> > No, thats completely wrong. When you look on assembly when using __builtin_isnan
> > then its identical to one of (on x64 but I doubt that arm gcc is worse)
> > 
> > __builtin_fpclassify (FP_NAN, FP_INFINITE, FP_NORMAL, FP_SUBNORMAL, FP_ZERO, x),0) == FP_NAN
> > 
> > So removing fpclassify wouldn't in better case didn't change performance
> > at all, in worse one it would harm it due to duplicated checks.
> 
> Fpclassify basically does several checks if you save the result in a variable and
> executes some branches. So you are far better off using dedicated checks if you
> just need 2 or 3 of the 5 possible results. And depending on how the code is
> structured you may only ever execute 1 check. That is far cheaper than first
> computing the full result for fpclassify and then testing that.
> 
Which doesn't matter. As gcc optimized unneded checks away you won't do
unneeded checks. As using:

__builtin_fpclassify (FP_NAN, FP_INFINITE,           \
     FP_NORMAL, FP_SUBNORMAL, FP_ZERO, x),0);
  return result == FP_INFINITE || result == FP_NAN;

is slower than:

 return __builtin_isinf (x) ||  __builtin_isnan (x);

Your claim is false, run attached tf2 script to test.

conditional add
isnan+isinf

real	0m0.141s
user	0m0.140s
sys	0m0.000s
classify

real	0m0.136s
user	0m0.132s
sys	0m0.000s
branch
isnan+isinf

real	0m0.158s
user	0m0.156s
sys	0m0.000s
classify

real	0m0.136s
user	0m0.132s
sys	0m0.000s
sum
isnan+isinf

real	0m0.237s
user	0m0.236s
sys	0m0.000s
classify

real	0m0.202s
user	0m0.200s
sys	0m0.000s




> > > A run of the math tests doesn't show up any obvious differences beyond the
> > > usual variations from run to run. I suspect the difference due to inlining
> > > is in the noise for expensive math functions.
> > >
> > Look at complex math, these use it. For real math you need to pick
> > specific inputs to trigger unlikely path that uses isinf...
> 
> Yes this needs a dedicated test. Still if we save a cycle in a 100 cycle function,
> it is hard to show it given modern OoO CPUs can have 5% variation from run to run...
> 
It isn't hard. You just need to run it long enough until variance is
below one cycle. Also with OoO its even more important as while function
could be slower on benchmark it could be faster in practice as it takes
advantage of that and selects instructions that are easy to OoO so most
happens in parallel.

Attachment: ft.c
Description: Text document

Attachment: ft2.c
Description: Text document

Attachment: tf
Description: Text document

Attachment: tf2
Description: Text document