This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: V2 [PATCH] x86: Use _rdtsc intrinsic for HP_TIMING_NOW
On 03/10/2018 20:08, H.J. Lu wrote:
> On Tue, Oct 2, 2018 at 11:06 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Tue, Oct 2, 2018 at 10:23 AM Adhemerval Zanella
>> <adhemerval.zanella@linaro.org> wrote:
>>>
>>>
>>> On 02/10/2018 13:56, H.J. Lu wrote:
>>>> On Tue, Oct 2, 2018 at 6:45 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On Tue, Oct 2, 2018 at 6:15 AM Adhemerval Zanella
>>>>> <adhemerval.zanella@linaro.org> wrote:
>>>>>> 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.
>>>> Here is the V2 patch. OK for master?
>>>>
>>>> NB: Checking if __i686__ isn't sufficient since __i686__ may not be
>>>> defined when building for i686 class processors.
>>> Right, it seems gcc does not define it for -march newer than pentium4.
>>>
>>>> diff --git a/sysdeps/i386/i586/init-arch.h b/sysdeps/i386/i586/isa.h
>>>> similarity index 89%
>>>> rename from sysdeps/i386/i586/init-arch.h
>>>> rename to sysdeps/i386/i586/isa.h
>>>> index 72fb46c61e..a2b5d585d4 100644
>>>> --- a/sysdeps/i386/i586/init-arch.h
>>>> +++ b/sysdeps/i386/i586/isa.h
>>>> @@ -1,4 +1,4 @@
>>>> -/* Copyright (C) 2015-2018 Free Software Foundation, Inc.
>>>> +/* Copyright (C) 2018 Free Software Foundation, Inc.
>>>> This file is part of the GNU C Library.
>>>>
>>>> The GNU C Library is free software; you can redistribute it and/or
>>>> @@ -15,5 +15,9 @@
>>>> License along with the GNU C Library; if not, see
>>>> <http://www.gnu.org/licenses/>. */
>>>>
>>>> +#ifndef _isa_h
>>>> +#define _isa_h
>>>> +
>>> Not sure if this is explicit on our coding rules, but usually we
>>> use uppercase caps for preprocessor guards. I think it requires
>>> a one-line description as well (same for other isa.h files).
>> I will change that.
>>
>>>> -#ifndef _HP_TIMING_H
>>>> -#define _HP_TIMING_H 1
>>>> +#include <isa.h>
>>>> +
>>>> +#if MINIMUM_ISA == 686 || MINIMUM_ISA == 8664
>>>> +# ifndef _HP_TIMING_H
>>>> +# define _HP_TIMING_H 1
>>>> +
>>> The include guard inside the MINIMUM_ISA check seems off, why do
>>> you need it?
>> <sysdeps/generic/hp-timing.h> will be skipped if _HP_TIMING_H is
>> defined. That is the draw back for the single x86 hp-timing.h.
> Here is the updated patch. OK for master?
LGTM.
> index 59af526fdb..1c20e9d828 100644
> --- a/sysdeps/i386/i686/hp-timing.h
> +++ b/sysdeps/x86/hp-timing.h
> @@ -1,7 +1,6 @@
> -/* High precision, low overhead timing functions. i686 version.
> - Copyright (C) 1998-2018 Free Software Foundation, Inc.
> +/* High precision, low overhead timing functions. x86 version.
> + Copyright (C) 2018 Free Software Foundation, Inc.
> This file is part of the GNU C Library.
> - Contributed by Ulrich Drepper <drepper@cygnus.com>, 1998.
>
> The GNU C Library is free software; you can redistribute it and/or
> modify it under the terms of the GNU Lesser General Public
> @@ -20,12 +19,17 @@
> #ifndef _HP_TIMING_H
> #define _HP_TIMING_H 1
Another possibility is define _HP_TIMING_X86_1 and not undefine it below,
I don't have a strong opinion, so either way look good to me.