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

Message ID CAMe9rOo3m_x-EpY2w=ZzbLDd2MjjLhrBY6fP-ngDFGDG-Z+94A@mail.gmail.com
State New, archived
Headers

Commit Message

H.J. Lu March 11, 2016, 10:41 p.m. UTC
  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.
  

Comments

Florian Weimer March 11, 2016, 10:49 p.m. UTC | #1
* 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. UTC | #2
> 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. UTC | #3
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