Patchwork [1/2] Add _arch_/_cpu_ to index_*/bit_* in x86 cpu-features.h

login
register
mail settings
Submitter H.J. Lu
Date March 11, 2016, 10:41 p.m.
Message ID <CAMe9rOo3m_x-EpY2w=ZzbLDd2MjjLhrBY6fP-ngDFGDG-Z+94A@mail.gmail.com>
Download mbox | patch
Permalink /patch/11309/
State New
Headers show

Comments

H.J. Lu - March 11, 2016, 10:41 p.m.
On Fri, Mar 11, 2016 at 2:29 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> On Fri, Mar 11, 2016 at 2:00 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> >> Sorry I didn't mention that I tested it on both x86-64 and i686 before
>> >> commit.
>> >
>> > But clearly you didn't!  The tree right now is broken, as I said.
>>
>> I double checked again with
>>
>> commit 6aa3e97e2530f9917f504eb4146af119a3f27229
>> Author: H.J. Lu <hjl.tools@gmail.com>
>> Date:   Thu Mar 10 05:26:46 2016 -0800
>>
>>     Add _arch_/_cpu_ to index_*/bit_* in x86 cpu-features.h
>>
>> on x86-64 and i686.  I only see:
>>
>> [hjl@gnu-6 build-i686-linux]$ ./nptl/tst-cleanupx4
>> test 0
>> clh (1)
>> clh (2)
>> clh (3)
>> test 1
>> clh (1)
>> clh (4)
>> clh (5)
>> clh (6)
>> test 2
>> clh (7)
>> clh (8)
>> global = 64, expected 120
>> test 3
>> clh (1)
>> clh (2)
>> clh (9)
>> clh (10)
>> [hjl@gnu-6 build-i686-linux]$
>>
>> Did you remove the old build directory?
>
> http://130.211.48.148:8080/builders/glibc-x86_64-linux/builds/1125/steps/check%20%28clobber%29/logs/stdio

The error is

../sysdeps/x86_64/tst-audit10.c: In function ‘avx512_enabled’:
../sysdeps/x86_64/tst-audit10.c:34:15: error: ‘bit_AVX512F’ undeclared
(first use in this function)
   if (!(ebx & bit_AVX512F))

which is changed by

ommit 3c0f7407eedb524c9114bb675cd55b903c71daaa
Author: Florian Weimer <fweimer@redhat.com>
Date:   Mon Mar 7 16:00:25 2016 +0100

    tst-audit4, tst-audit10: Compile AVX/AVX-512 code separately [BZ #19269]

    This ensures that GCC will not use unsupported instructions before
    the run-time check to ensure support.

bit_AVX512F is defined in <cpuid.h>  from GCC.  Apparently, your GCC
doesn't have it.  You can try this patch.
Florian Weimer - March 11, 2016, 10:49 p.m.
* H. J. Lu:

> ../sysdeps/x86_64/tst-audit10.c: In function ‘avx512_enabled’:
> ../sysdeps/x86_64/tst-audit10.c:34:15: error: ‘bit_AVX512F’ undeclared
> (first use in this function)
>    if (!(ebx & bit_AVX512F))
>
> which is changed by
>
> ommit 3c0f7407eedb524c9114bb675cd55b903c71daaa
> Author: Florian Weimer <fweimer@redhat.com>
> Date:   Mon Mar 7 16:00:25 2016 +0100
>
>     tst-audit4, tst-audit10: Compile AVX/AVX-512 code separately [BZ #19269]
>
>     This ensures that GCC will not use unsupported instructions before
>     the run-time check to ensure support.
>
> bit_AVX512F is defined in <cpuid.h>  from GCC.  Apparently, your GCC
> doesn't have it.  You can try this patch.

Roland committed a fix which included <cpu-features.h> (commit
3bd80c0de2f8e7ca8020d37739339636d169957e) in tst-audit10.c I assumed
that this addressed this particular build failure (it does for me with
GCC 4.7).

Ah, you removed that flag in 6aa3e97e2530f9917f504eb4146af119a3f27229.
So you need to adjust tst-audit10 for this change.

> -tests += tst-audit3 tst-audit4 tst-audit5 tst-audit6 tst-audit7 tst-audit10
> +tests += tst-audit3 tst-audit4 tst-audit5 tst-audit6 tst-audit7
> +
> +ifeq (yes,$(config-cflags-avx512))
> +tests += tst-audit10
> +endif

The test should be marked as UNSUPPORTED instead.
Roland McGrath - March 11, 2016, 10:55 p.m.
> Roland committed a fix which included <cpu-features.h> (commit
> 3bd80c0de2f8e7ca8020d37739339636d169957e) in tst-audit10.c I assumed
> that this addressed this particular build failure (it does for me with
> GCC 4.7).

That fixed it on the bot too, which is GCC 4.8.4.

> Ah, you removed that flag in 6aa3e97e2530f9917f504eb4146af119a3f27229.
> So you need to adjust tst-audit10 for this change.

Indeed.

> > -tests += tst-audit3 tst-audit4 tst-audit5 tst-audit6 tst-audit7 tst-audit10
> > +tests += tst-audit3 tst-audit4 tst-audit5 tst-audit6 tst-audit7
> > +
> > +ifeq (yes,$(config-cflags-avx512))
> > +tests += tst-audit10
> > +endif
> 
> The test should be marked as UNSUPPORTED instead.

That is adequately covered inside the test already (at runtime).
There is no reason to conditionalize building it.
H.J. Lu - March 11, 2016, 11 p.m.
On Fri, Mar 11, 2016 at 2:55 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> Roland committed a fix which included <cpu-features.h> (commit
>> 3bd80c0de2f8e7ca8020d37739339636d169957e) in tst-audit10.c I assumed
>> that this addressed this particular build failure (it does for me with
>> GCC 4.7).

<cpu-features.h> shouldn't be used in test.  We can put cpuid.h
in incldue and do

#include_next <cpuid.h>

#ifndef bit_AVX512F
# define bit_AVX512F
#endif

It is better than use <cpu-features.h>.

Patch

diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
index aa4a754..2525cc1 100644
--- a/sysdeps/x86_64/Makefile
+++ b/sysdeps/x86_64/Makefile
@@ -47,7 +47,11 @@  tests-pie += $(quad-pie-test)
 $(objpfx)tst-quad1pie: $(objpfx)tst-quadmod1pie.o
 $(objpfx)tst-quad2pie: $(objpfx)tst-quadmod2pie.o

-tests += tst-audit3 tst-audit4 tst-audit5 tst-audit6 tst-audit7 tst-audit10
+tests += tst-audit3 tst-audit4 tst-audit5 tst-audit6 tst-audit7
+
+ifeq (yes,$(config-cflags-avx512))
+tests += tst-audit10
+endif

 tests += tst-split-dynreloc
 LDFLAGS-tst-split-dynreloc = -Wl,-T,$(..)sysdeps/x86_64/tst-split-dynreloc.lds