Re: [PATCH] x86: Use _rdtsc intrinsic for HP_TIMING_NOW

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Tue, Oct 2, 2018 at 6:15 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 01/10/2018 19:08, H.J. Lu wrote:
> > Since _rdtsc intrinsic is supported in GCC 4.9, we can use it for
> > HP_TIMING_NOW.
> >
> >       * sysdeps/i386/i686/hp-timing.h: Include <x86intrin.h>.
> >       (HP_TIMING_NOW): Use _rdtsc.
> >       * sysdeps/x86_64/hp-timing.h: Just include
> >       <sysdeps/i386/i686/hp-timing.h>.
> > ---
> >  sysdeps/i386/i686/hp-timing.h |  4 +++-
> >  sysdeps/x86_64/hp-timing.h    | 41 +----------------------------------
> >  2 files changed, 4 insertions(+), 41 deletions(-)
> >
> > diff --git a/sysdeps/i386/i686/hp-timing.h b/sysdeps/i386/i686/hp-timing.h
> > index 59af526fdb..6eee8c58be 100644
> > --- a/sysdeps/i386/i686/hp-timing.h
> > +++ b/sysdeps/i386/i686/hp-timing.h
> > @@ -20,6 +20,8 @@
> >  #ifndef _HP_TIMING_H
> >  #define _HP_TIMING_H 1
> >
> > +#include <x86intrin.h>
> > +
> >  /* We always assume having the timestamp register.  */
> >  #define HP_TIMING_AVAIL              (1)
> >  #define HP_SMALL_TIMING_AVAIL        (1)
> > @@ -35,7 +37,7 @@ typedef unsigned long long int hp_timing_t;
> >     running in this moment.  This could be changed by using a barrier like
> >     'cpuid' right before the `rdtsc' instruciton.  But we are not interested
> >     in accurate clock cycles here so we don't do this.  */
> > -#define HP_TIMING_NOW(Var)   __asm__ __volatile__ ("rdtsc" : "=A" (Var))
> > +#define HP_TIMING_NOW(Var)   ({ (Var) = _rdtsc (); })
>
> Do we need a compound statement here? It does not use loops, switches, or
> local variables.

I have a followup patch to use _rdtscp which will need a local variable.

> >
> >  #include <hp-timing-common.h>
> >
> > diff --git a/sysdeps/x86_64/hp-timing.h b/sysdeps/x86_64/hp-timing.h
> > index ec543bef03..46236b9777 100644
> > --- a/sysdeps/x86_64/hp-timing.h
> > +++ b/sysdeps/x86_64/hp-timing.h
> > @@ -1,40 +1 @@
>
> [...]
>
> > -
> > -#endif /* hp-timing.h */
> > +#include <sysdeps/i386/i686/hp-timing.h>
> >
>
> I am not very found of cross abi includes like this one, wouldn't be better
> to create a 'sysdeps/x86/hp-timing.h' and for 32-bit use _rdtsc iff
> __i686__ is defined otherwise include generic header?

I can do that.

-- 
H.J.