From: "H.J. Lu" <hjl.tools@gmail.com>
To: Sriraman Tallam <tmsriram@google.com>
Cc: Uros Bizjak <ubizjak@gmail.com>,
Richard Guenther <richard.guenther@gmail.com>,
Michael Matz <matz@suse.de>,
reply@codereview.appspotmail.com, gcc-patches@gcc.gnu.org,
Richard Henderson <rth@redhat.com>, Jan Hubicka <jh@suse.cz>
Subject: Re: Support for Runtime CPU type detection via builtins (issue5754058)
Date: Mon, 23 Apr 2012 20:41:00 -0000 [thread overview]
Message-ID: <CAMe9rOrw7bxJDCQyJxkT_89QqL6SVOGkp5N1AFZo7ukgFLLc1g@mail.gmail.com> (raw)
In-Reply-To: <CAAs8HmxahQ_5S4ktpi_J54SMdwEVNa-7scfgnHSBO=ndTNpf6w@mail.gmail.com>
On Mon, Apr 23, 2012 at 9:59 AM, Sriraman Tallam <tmsriram@google.com> wrote:
> Hi,
>
> On Mon, Apr 23, 2012 at 1:19 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Tue, Apr 3, 2012 at 9:47 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.
>
>
>>
>>>>>> Patch available for review here:
>>>>>> http://codereview.appspot.com/5754058
>>
>> Please attach patches or inline it in the message itself for a review.
>> Please see [1] for further instructions.
>
> Patch attached, tested on x86_64 and all tests pass.
>
>
> * 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.
>
> Thanks,
> -Sri.
>
>
I have 2 comments:
1. You should remove
static int called = 0;
if (called)
return 0;
else
called = 1;
Instead, you can just do
if (_cpu_model.__cpu_vendor)
return 0;
2. You can replace
if (vendor == SIG_AMD)
with
else if (vendor == SIG_AMD)
Thanks.
--
H.J.
next prev parent reply other threads:[~2012-04-23 20:41 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-03-30 0:10 Sriraman Tallam
2012-03-30 12:47 ` Michael Matz
2012-03-30 23:03 ` Sriraman Tallam
2012-04-02 12:38 ` Richard Guenther
2012-04-02 18:00 ` Sriraman Tallam
2012-04-03 0:48 ` Sriraman Tallam
2012-04-03 19:47 ` Sriraman Tallam
2012-04-12 23:15 ` Sriraman Tallam
2012-04-18 23:09 ` Sriraman Tallam
2012-04-23 8:20 ` Uros Bizjak
2012-04-23 16:59 ` Sriraman Tallam
2012-04-23 19:17 ` Uros Bizjak
2012-04-23 20:43 ` Sriraman Tallam
2012-04-24 0:46 ` H.J. Lu
2012-04-25 0:10 ` Sriraman Tallam
2012-04-25 0:24 ` H.J. Lu
2012-04-25 2:07 ` Sriraman Tallam
2012-04-25 2:39 ` H.J. Lu
2012-04-25 21:25 ` Sriraman Tallam
2012-04-25 21:28 ` H.J. Lu
2012-04-25 21:45 ` Sriraman Tallam
2012-04-25 23:38 ` Sriraman Tallam
2012-04-25 23:52 ` H.J. Lu
2012-04-26 0:52 ` Sriraman Tallam
2012-06-05 23:46 ` Sriraman Tallam
2012-06-06 14:03 ` H.J. Lu
2012-06-12 3:50 ` Sriraman Tallam
2012-04-26 6:10 ` Marc Glisse
2012-04-23 19:31 ` Uros Bizjak
2012-04-23 20:41 ` H.J. Lu [this message]
2012-04-23 17:04 ` Sriraman Tallam
2012-04-08 13:18 ` Gerald Pfeifer
2012-04-09 17:19 ` Sriraman Tallam
-- strict thread matches above, loose matches on Subject: below --
2012-03-07 0:49 Sriraman Tallam
2012-03-07 13:51 ` Richard Guenther
2012-03-08 20:35 ` Xinliang David Li
2012-03-12 11:17 ` Richard Guenther
2012-03-30 0:13 ` Sriraman Tallam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMe9rOrw7bxJDCQyJxkT_89QqL6SVOGkp5N1AFZo7ukgFLLc1g@mail.gmail.com \
--to=hjl.tools@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jh@suse.cz \
--cc=matz@suse.de \
--cc=reply@codereview.appspotmail.com \
--cc=richard.guenther@gmail.com \
--cc=rth@redhat.com \
--cc=tmsriram@google.com \
--cc=ubizjak@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).