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: "Wilco Dijkstra" <wdijkstr at arm dot com>
- To: 'Ondřej Bílka' <neleai at seznam dot cz>, "Carlos O'Donell" <carlos at redhat dot com>
- Cc: "'Siddhesh Poyarekar'" <siddhesh at redhat dot com>, "'GNU C Library'" <libc-alpha at sourceware dot org>
- Date: Tue, 21 Jul 2015 17:17:23 +0100
- 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> <20150721113502 dot GA3415 at domone>
> 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