On Mon, Apr 23, 2012 at 1:43 PM, Sriraman Tallam <tmsri...@google.com> wrote: > On Mon, Apr 23, 2012 at 12:16 PM, Uros Bizjak <ubiz...@gmail.com> wrote: >> On Mon, Apr 23, 2012 at 6:59 PM, Sriraman Tallam <tmsri...@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.