This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Support for Runtime CPU type detection via builtins (issue5754058)


Hi,

   Thanks for all the comments. I have made all the changes as
mentioned and submiited the patch. Summary of changes made:

* Add support for AVX
* Fix documentation in extend.texi
* Make it thread-safe according to H.J.'s comments.

I have attached the patch. Boot-strapped and checked for test parity
with pristine build.

       * config/i386/i386.c (build_processor_model_struct): New function.
        (make_var_decl): New function.
        (fold_builtin_cpu): New function.
        (ix86_fold_builtin): New function.
        (make_cpu_type_builtin): New function.
        (ix86_init_platform_type_builtins): New function.
        (ix86_expand_builtin): Expand new builtins by folding them.
        (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 symbol __cpu_model.


Thanks,
-Sri.

On Mon, Apr 23, 2012 at 5:46 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> 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.

Attachment: cpu_runtime_patch.txt
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]