Patchwork [wwwdocs] Document Runtime CPU detection builtins

Hi Gerald / Diego,

    I have made all the mentioned changes.  I also shortened the
description like Diego mentioned by removing all the strings but kept
the caveats. I have not added a reference to the documentation because
i do not know what link to reference. The builtins are completely
documented in extend.texi.

   I have attached the patch. If there are no further comments I will
submit this tomorrow.

Thanks,
-Sri.


On Mon, Aug 20, 2012 at 1:31 PM, Gerald Pfeifer <gerald@pfeifer.com> wrote:
> Hi Sriraman,
>
> On Fri, 10 Aug 2012, Sriraman Tallam wrote:
>> I have added a release note for x86 builtins __builtin_cpu_is and
>> __builtin_cpu_supports. They were checked in to trunk in rev. 186789.
>
> I had hoped one of the x86 maintainers would review this from his
> perspective given that they have more background.  For the lack of
> that, let me give it a try.
>
> Index: changes.html
> ===================================================================
> +    <li> New builtin functions to detect run-time CPU type and ISA:<br>
>
> "built-in", cf. http://gcc.gnu.org/codingconventions.html; here and
> in the following.
>
> No <br> here; <ul> should just do that.
>
> +    <ul>
> +      <li>Builtin <code>__builtin_cpu_is</code> has been added to detect if
> +      the run-time CPU is of a particular type. The builtin returns a postive
> +      integer on a match and zero otherwise. The builtin accepts one string
> +      literal argument, the CPU name. For example,
>
> "A built-in function..."
>
> "positive"
>
> "It accepts one string" (to make this shorter)
>
> +      <code>__builtin_cpu_is("westmere")</code> returns a postive integer if
>
> "positive"
>
> +      the run-time CPU is an Intel Corei7 Westmere processor.  The following
>
> I don't work for Intel, but should there be a space before "i7"?
>
> +      are the CPU names recognized by <code>__builtin_cpu_is:</code>
>
> How about making this "The following are the CPU names recognized for
> now", which avoids another reference to the name of the built-in and
> makes it clear that this is subject to change.
>
> +      <li>Builtin <code>__builtin_cpu_supports</code> has been added to detect
>
> "A built-in function..."
>
> +      returns a postive integer on a match and zero otherwise. The builtin
>
> "positive"
>
> +      following are the ISA features recognized by
> +      <code>__builtin_cpu_supports:</code>
>
> Same is above?
>
> +    <p>Caveat: If the above builtins are called before any constructors are
> +    invoked, like during IFUNC initialization, then the CPU detection
> +    initialization must be explicity run using this newly provided
> +    builtin,  <code>__builtin_cpu_init</code>.
>
> "...using the new built-in function <code>__builtin_cpu_init</code>."
>
> What is a constructor in this context, by the way?  Will this be clear
> to all the users?
>
> +    <code>
> +    static void (*some_ifunc_resolver(void))(void)<br>
> +    {<br>
> +    &nbsp&nbsp __builtin_cpu_init();<br>
> +    &nbsp&nbsp if (__builtin_cpu_is("amdfam10h") ...<br>
> +    &nbsp&nbsp if (__builtin_cpu_supports("popcnt") ...<br>
> +    }
> +    </code>
>
> How about using <pre> here? That avoids the <br/>s which will cause
> problems with the web page validator, by the way.
>
>
> Nice job for documenting this so well.  Thanks for taking the time
> and your patience!
>
> The patch is fine modulo the changes I pointed out (though some of
> them are more suggestions and you do not need to slavishly follow
> those).
>
> Gerald

On 2012-08-20 22:41 , Sriraman Tallam wrote:
> Hi Gerald / Diego,
>
>      I have made all the mentioned changes.  I also shortened the
> description like Diego mentioned by removing all the strings but kept
> the caveats. I have not added a reference to the documentation because
> i do not know what link to reference. The builtins are completely
> documented in extend.texi.

Referring to the user's manual is OK, I think.

> +    <p>Caveat: If these built-in functions are called before any static
> +    constructors are invoked, like during IFUNC initialization, then the CPU
> +    detection initialization must be explicity run using this newly provided

s/explicity/explicitly/

Other than that, it looks fine to me.


Diego.

Committed after making the changes.

One small problem, I am not sure how to fix this:

The hyper link I referenced is :
http://gcc.gnu.org/onlinedocs/gcc/X86-Built_002din-Functions.html#X86-Built_002din-Functions

whereas the committed changes.html is pointing to:
http://gcc.gnu.org/onlinedocs/gcc/X86-Built-in-Functions.html#X86-Built-in-Functions

Please note that the "_002din" is missing. This makes the link broken,
did I miss anything? I verified that I submitted the right link.

Thanks,
-Sri.

On Tue, Aug 21, 2012 at 5:41 AM, Diego Novillo <dnovillo@google.com> wrote:
> On 2012-08-20 22:41 , Sriraman Tallam wrote:
>>
>> Hi Gerald / Diego,
>>
>>      I have made all the mentioned changes.  I also shortened the
>> description like Diego mentioned by removing all the strings but kept
>> the caveats. I have not added a reference to the documentation because
>> i do not know what link to reference. The builtins are completely
>> documented in extend.texi.
>
>
> Referring to the user's manual is OK, I think.
>
>> +    <p>Caveat: If these built-in functions are called before any static
>> +    constructors are invoked, like during IFUNC initialization, then the
>> CPU
>> +    detection initialization must be explicity run using this newly
>> provided
>
>
> s/explicity/explicitly/
>
> Other than that, it looks fine to me.
>
>
> Diego.