flac dev - Dec 2016 - Q: test for CPUID instruction presence

I found that FLAC__cpu_have_cpuid_x86() was removed in the commit
<http://git.xiph.org/?p=flac.git;a=commitdiff;h=fa24613ad94ba8fb8e23bcb9ca80b4548bb617e6>
with the message:
  "Remove `FLAC__cpu_have_cpuid_x86` altogether as it wasn't actually
   being used but that was difficult to tell because of all the #ifdef
   nonsense."

But FLAC__cpu_have_cpuid_x86() actually WAS used in the code

   #if !defined FLAC__NO_ASM && (defined FLAC__HAS_NASM ||  
FLAC__HAS_X86INTRIN)
             info->use_asm = true; /* we assume a minimum of 80386 with  
FLAC__CPU_IA32 */
     #if FLAC__HAS_X86INTRIN
             if(!FLAC__cpu_have_cpuid_x86())
                     return;
     #else
             if(!FLAC__cpu_have_cpuid_asm_ia32())
                     return;
     #endif


I'll try to explain it:

   #if !defined FLAC__NO_ASM && (defined FLAC__HAS_NASM ||  
FLAC__HAS_X86INTRIN)
             /* FLAC__HAS_NASM is defined,
                             or FLAC__HAS_X86INTRIN is set to 1, or both */
             /* Now we want to test availability of CPUID instruction: */
     #if FLAC__HAS_X86INTRIN
             /* FLAC__cpu_have_cpuid_x86() is available, and we call it */
     #else
             /* FLAC__HAS_X86INTRIN is 0, this means that FLAC__HAS_NASM is  
defined
               so we can use asm functions, and we call  
FLAC__cpu_have_cpuid_asm_ia32() */
     #endif


Currently libFLAC doesn't check the existence of CPUID instruction if
FLAC__HAS_X86INTRIN is set to 1.
It's not a real problem because x86 CPUs without CPUID are probably
extinct, but if libFLAC performs this check then it should do it
in all cases (when NASM is available or intrinsics are available).

lvqcl.mail wrote:
> Currently libFLAC doesn't check the existence of CPUID instruction if
> FLAC__HAS_X86INTRIN is set to 1.
> It's not a real problem because x86 CPUs without CPUID are probably
> extinct, but if libFLAC performs this check then it should do it
> in all cases (when NASM is available or intrinsics are available).
This code in cpu.c has been a mess for over a decade. Forunately the
need for complexity is decreasing.

I propose that we release FLAC with cpu.c as it is now and deal with
the consequences as they arise.

Erik
-- 
----------------------------------------------------------------------
Erik de Castro Lopo
http://www.mega-nerd.com/

Erik de Castro Lopo wrote:
> lvqcl.mail wrote:
>
>> Currently libFLAC doesn't check the existence of CPUID instruction
if
>> FLAC__HAS_X86INTRIN is set to 1.
>> It's not a real problem because x86 CPUs without CPUID are probably
>> extinct, but if libFLAC performs this check then it should do it
>> in all cases (when NASM is available or intrinsics are available).
>
> This code in cpu.c has been a mess for over a decade. Forunately the
> need for complexity is decreasing.
>
> I propose that we release FLAC with cpu.c as it is now and deal with
> the consequences as they arise.
Makes sense.

But still... IMHO it makes sense to change "if !FLAC__HAS_X86INTRIN"
to "if defined FLAC__HAS_NASM". It covers more cases, and it's
logical:
FLAC__cpu_have_cpuid_asm_ia32() can be called if (and only if) NASM is  
available.
(the patch is attached just in case).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: have_cpuid.patch
Type: application/octet-stream
Size: 456 bytes
Desc: not available
URL:
<http://lists.xiph.org/pipermail/flac-dev/attachments/20161205/b52dad2d/attachment.obj>