RE: [PATCH v2] Add math-inline benchmark

["Wilco Dijkstra" <wdijkstr at arm dot com>]

> Ondřej Bílka wrote:
> On Mon, Jul 20, 2015 at 03:23:50PM -0400, Carlos O'Donell wrote:
> > On 07/20/2015 12:38 PM, Wilco Dijkstra wrote:
> > >> Siddhesh Poyarekar wrote:
> > >> > On Fri, Jul 17, 2015 at 09:49:42AM -0400, Carlos O'Donell wrote:
> > >>> > > Maybe we can place this in another C file e.g. bench-util.c and #include that
> > >>> > > and then call those functions?
> > >> >
> > >> > Yes, that is fine.
> > > I've extracted the start function in bench-util.c, see updated version.
> > >
> > > Wilco
> >
> > One nit, OK to commit with that fixed.
> >
> No Carlos, this isn't ok. You need to do better review as this patch has
> still some issues.

I don't think there is any point in blaming Carlos.

> I am not talking about problems that this doesn't measure real workload
> at all but simple problems that reviewer should point out.
> 
> Here its that builtins were added but not well. As this doesn't have
> builtin_fpclassify comparing these doesn't make sense.
> 
> 
> > > +  int cl = fpclassify (d);
> > > +  return cl == FP_NAN || cl == FP_INFINITE;
> 
> and
> 
> > > +  return __builtin_isnan (d) || __builtin_isinf (d);

Fpclassify uses the builtin, but I think it's better to remove these
tests as they don't add much value. It's pretty obvious it is better
to use separate tests instead of fpclassify when you only need 2 of them.

> Also you don't test inlines in isnormal:
> 
> > > +/* Explicit inline similar to existing math.h implementation.  */
> > > +
> > > +#define __isnormal_inl(X) (__fpclassify (X) == FP_NORMAL)

This is an inline and exactly what the current math.h does. The goal of my
benchmark is comparing the existing implementation with the new inlines.

Wilco