This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] Add math-inline benchmark
- From: OndÅej BÃlka <neleai at seznam dot cz>
- To: Carlos O'Donell <carlos at redhat dot com>
- Cc: Wilco Dijkstra <wdijkstr at arm dot com>, 'Siddhesh Poyarekar' <siddhesh at redhat dot com>, 'GNU C Library' <libc-alpha at sourceware dot org>
- Date: Tue, 21 Jul 2015 13:35:02 +0200
- Subject: Re: [PATCH v2] Add math-inline benchmark
- Authentication-results: sourceware.org; auth=none
- References: <002001d0bfb8$b36fa330$1a4ee990$ at com> <55A907F6 dot 8000504 at redhat dot com> <20150717143406 dot GG19592 at spoyarek dot pnq dot redhat dot com> <002c01d0c30a$8f23c600$ad6b5200$ at com> <55AD4AC6 dot 5070401 at redhat dot com>
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 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);
Also you don't test inlines in isnormal:
> > +/* Explicit inline similar to existing math.h implementation. */
> > +
> > +#define __isnormal_inl(X) (__fpclassify (X) == FP_NORMAL)