Support for Runtime CPU type detection via builtins (issue5754058)

[H.J. Lu]

On Mon, Apr 23, 2012 at 1:43 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Mon, Apr 23, 2012 at 12:16 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Mon, Apr 23, 2012 at 6:59 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>
>>>>> i386 maintainers -  Is this patch ok?
>>>>
>>>> Has the community reached the consensus on how this kind of
>>>> functionality has to be implemented? I have followed the discussion a
>>>> bit, but IIRC, there was no clear decision. Without this decision, I
>>>> am not able to review the _implementation_ of agreed functionality for
>>>> x86 target.
>>>>
>>>> (I apologize if I have missed the decision, please point me to the
>>>> discussion in this case.)
>>>
>>> The discussions are here:
>>>
>>> http://gcc.gnu.org/ml/gcc-patches/2011-08/msg01446.html
>>> and follow-ups to this.
>>>
>>> I am not sure about consensus, but the important points raised were:
>>>
>>> 1) Constructor ordering: What if some constructors fire before
>>> cpu_indicator_init?, which determines the CPU. I addressed this
>>> problem by making the priority of cpu_indicator_init to be the highest
>>> possible. Still, IFUNC initializers will fire before and they have to
>>> explicitly call __builtin_cpu_init() before checking the CPU type.
>>> 2) Reducing the number of builtins : It is only two now.
>>
>>>       * config/i386/i386.c (build_processor_features_struct): New function.
>>>       (build_processor_model_struct): New function.
>>>       (make_var_decl): New function.
>>>       (get_field_from_struct): New function.
>>>       (fold_builtin_target): New function.
>>>       (ix86_fold_builtin): New function.
>>>       (ix86_expand_builtin): Expand new builtins by folding them.
>>>       (make_cpu_type_builtin): New functions.
>>>       (ix86_init_platform_type_builtins): Make the new builtins.
>>>       (ix86_init_builtins): Make new builtins to detect CPU type.
>>>       (TARGET_FOLD_BUILTIN): New macro.
>>>       (IX86_BUILTIN_CPU_INIT): New enum value.
>>>       (IX86_BUILTIN_CPU_IS): New enum value.
>>>       (IX86_BUILTIN_CPU_SUPPORTS): New enum value.
>>>       * config/i386/i386-builtin-types.def: New function type.
>>>       * testsuite/gcc.target/builtin_target.c: New testcase.
>>>       * doc/extend.texi: Document builtins.
>>>
>>>       * libgcc/config/i386/i386-cpuinfo.c: New file.
>>>       * libgcc/config/i386/t-cpuinfo: New file.
>>>       * libgcc/config.host: Include t-cpuinfo.
>>>       * libgcc/config/i386/libgcc-glibc.ver: Version symbols __cpu_model
>>>       and __cpu_features.
>>
>> The patch is OK.
>>
>> I guess that AVX is left as an exercise for a x86 maintainer ;)
>
> I will add AVX, and make all the changes.
>

3 more comments:

1.  Your implementation isn't thread-safe. With my suggestion of

if (_cpu_model.__cpu_vendor)
 return 0;

you should change get_intel_cpu to return processor_vendor and
set _cpu_model.__cpu_vendor after setting up all other bits.

2. Bitfields

struct __processor_features
+{
+  unsigned int __cpu_cmov : 1;
+  unsigned int __cpu_mmx : 1;
+  unsigned int __cpu_popcnt : 1;
+  unsigned int __cpu_sse : 1;
+  unsigned int __cpu_sse2 : 1;
+  unsigned int __cpu_sse3 : 1;
+  unsigned int __cpu_ssse3 : 1;
+  unsigned int __cpu_sse4_1 : 1;
+  unsigned int __cpu_sse4_2 : 1;
+} __cpu_features;

may not be thread-safe since only char/short/int/long long
stores are atomic and it isn't extensible. You can use

unsigned int __cpu_features[1];

to make it extensible and store atomic.

3. You may move

unsigned int __cpu_features[1];

into struct __processor_model to only export one symbol
instead of 2.

-- 
H.J.