Adding doxygen comments to the LLVM intrinsics (part 1, ammintrin.h)

[kromanova]

Summary

Hello,

About a week ago I have started a new discussion about adding comments to x86 intrinsics headers.
http://permalink.gmane.org/gmane.comp.compilers.clang.devel/42032

Here is the first small patch providing doxygen comments for all the intrinsics in ammintrin.h.
How does it look?

Katya.

[kromanova]

Will it be possible to review/approve the format of the doxygen comments for x86 intrinsics? When the format is approved, I could add doxygen comments for 800 intrinsics (one file at a time).

Also, please review the first patch.

Thank you!

[gribozavr]

ammintrin.h

44 ↗

(On Diff #23033)

This *intrinsic* (here and everywhere).

50 ↗

(On Diff #23033)

It would help to explicitly say something about other bits: are they ignored, or are they required to be zero?

Also, the word 'length' does not have the right connotation, I think. 'Bits [5:0] specify the number of bits to extract, other bits are ignored' -- I think something like this would be much better.

102 ↗

(On Diff #23033)

'Immediate' does not mean much at the C level. Should it be a literal? A constant expression?

[kromanova]

Sorry for the delay. A technical writer had to be involved. Unfortunately, all the content-related changes (vs. changes in the doxygen tool generator itself that I totally control) will take a long time.

Addressed Dmitri's comments. The only thing that I left "as is" is a length of a bitfield. I looked up MS documentation for these intrinsics. The word 'length' used all the time.

Also, the word 'length' does not have the right connotation, I think. 'Bits [5:0] specify the number of bits to extract, other bits are ignored' -- I think something like this would be much better.

[gribozavr]

LGTM.

A technical writer had to be involved. Unfortunately, all the content-related changes (vs. changes in the doxygen tool generator itself that I totally control) will take a long time.

You realize that once you commit these comments to header files, other people can (and would) edit them in a way that might be incompatible with your documentation guidelines, right?

[kromanova]

Thanks for the review Dmitri! My concern is that this header file is only the tip of the iceberg. We still have 800 more intrinsic comments to go. Hopefully, there will be someone else who could help with the reviews. I'm afraid to burden you.

You realize that once you commit these comments to header files, other people can (and would) edit them in a way that might be incompatible with your documentation guidelines, right?

Yep. That's the goal. We will have a post-run formatting script that will take care of all incompatibilities. The only hope is that all format-related incompatibilities will be consistent in the future. If format changes happens, we will have to adjust our post-running script to continue generate the documentation that we want. I know that it will be probably quite hard to enforce the consistency for intrinsics comments (unless -Wdocumentation could do it automatically :) ). When we are merging with upstream, we will review all doxygen comments changes and hopefully catch all the entries with the invalid format (because either our script will complain or the diff for our documentation will look suspicious). Also, I hope that all content related changes in ToT will be for the best. If not, again, hopefully our tech writers will catch the problems during review and we will fix or report them.

I will rebase and commit. I will start preparing the review for the second small header tomorrow.

[kromanova]

Dmitri, one more thing. I have neither Mac nor Xcode to try out and see if documentation looks reasonable. There might be some problems caused by attribute((..)). Could you please check? I'd rather make sure that the documentation looks good before I commit. I will try to get access to Xcode as soon as I can, but it might not take a while, so could you please do this as one time favor?
Thank you!

[kromanova]

Hi Dmitri,

One of my colleagues has a Mac, so we have looked up how the documentation for ammintrin.h is rendered the XCode today.

Good news: Regular instrinsics definitions are displayed correctly, though some small changes might be needed to make rendering of the documentation better. I don’t personally like this “code snippet” produced by this comment

/ \code
/ This intrinsic corresponds to the \c VADDPD instruction.
/// \endcode

I don't care for the gray box surrounding this text, identifying it as a code snippet. I don't find it particularly pretty, though I could live with that. Do you have better ideas?
What I definitely don't like is \c command that you asked me to add some time back. It definitely looks weird in XCode. It's interpreted literally, not like a command. I will try to attach a screenshot with generated XCode documentation for one of the intrinsics to Phabricator, so you would know what I'm talking about. If it won't work, I will send you a personal email with the screenshot. Again, thank you so much for reviewing and making helpful comments!

Bad news: Macros are not getting rendered in XCode at all. To say more, unrelated to the intrinsics headers, I wasn't even able to display documentation for the simplest macro that adds two numbers together. Do you know if we could work around it?

[kromanova]

[gribozavr]

Macros are not getting rendered in XCode at all. To say more, unrelated to the intrinsics headers, I wasn't even able to display documentation for the simplest macro that adds two numbers together. Do you know if we could work around it?

This is a known limitation of Clang. It does not attach comments to macros.

Changing macro "intrinsics" into functions would be best.

lib/Headers/ammintrin.h

193–195

Why is this paragraph surrounded by \code? It is text, not program code.

[kromanova]

This is a known limitation of Clang. It does not attach comments to macros.
Changing macro "intrinsics" into functions would be best.

Changing macro intrinsics into functions is more invasive change than adding comments. It will affect a wide audience and might take a while to get accepted. I prefer to complete submitting all the doxygen documentation (for all of the header files) first and change the macros into the functions later.

[kromanova]

Why is this paragraph surrounded by \code? It is text, not program code.

I'm not sure if you remember it, but in the first draft of the doxygen comments format I had
/ \code
/ INSTRUCTION_NAME
/// \endcode
So in the first draft revision it was a program code.

In the review, you asked me to add "This intrinsic corresponds to the \c <INSTRUCTION_NAME> instruction" wording, which I did.
It's easy enough to change \code command into something else (what command does Clang supports and wants to see here?) or revert "This intrinsic corresponds to the \c .. instruction". Let me know what is preferable.
change back.

[jroelofs]

If you drop the \code ... \endcode around the "This intrinsic corresponds to the \c .. instruction" text, does it render better?

[jroelofs]

In fact, \remark might be the most appropriate tag to put it under.

[kromanova]

I will try out \remark and let you know. The only complication is that I don't have a Mac to try it out :)

[kromanova]

That's what being displayed with \remark

[kromanova]

That's what is being displayed without \remark (or \code) command around the instruction name. Which one do you prefer: with or without \remark command? I don't have a strong opinion about it. Both of them look acceptable to me.

[jroelofs]

I have a weak preference for \remark... I don't like it, but if there's a better one to throw it under, it will be easier to fix them all with sed than going the other way.

[jroelofs]

For avoidance of doubt: LGTM too.

[kromanova]

This was committed in r238386 but I forgot to close the review.

lib/Headers/ammintrin.h

 
 #include <pmmintrin.h>
 
+/// \brief Extracts the specified bits from the lower 64 bits of the 128-bit
+///    integer vector operand at the index idx and of the length len.
+///
+/// \headerfile <x86intrin.h>
+///
+/// \code 
+/// __m128i _mm_extracti_si64(__m128i x, const int len, const int idx);
+/// \endcode 
+///
+/// \code                                                    
+/// This intrinsic corresponds to the \c EXTRQ instruction.
+/// \endcode 
+///
+/// \param x
+///    The value from which bits are extracted.
+/// \param len
+///    Bits [5:0] specify the length; the other bits are ignored. If bits [5:0]
+///    are zero, the length is interpreted as 64.
+/// \param idx
+///    Bits [5:0] specify the index of the least significant bit; the other 
+///    bits are ignored. If the sum of the index and length is greater than 
+///    64, the result is undefined. If the length and index are both zero, 
+///    bits [63:0] of parameter x are extracted. If the length is zero 
+///    but the index is non-zero, the result is undefined.
+/// \returns A 128-bit integer vector whose lower 64 bits contain the bits
+///    extracted from the source operand.
 #define _mm_extracti_si64(x, len, idx) \
   ((__m128i)__builtin_ia32_extrqi((__v2di)(__m128i)(x), \
                                   (char)(len), (char)(idx)))
 
+/// \brief Extracts the specified bits from the lower 64 bits of the 128-bit
+///    integer vector operand at the index and of the length specified by __y.
+///
+/// \headerfile <x86intrin.h>
+///
+/// \code 
+/// This intrinsic corresponds to the \c EXTRQ instruction.
+/// \endcode 
+///
+/// \param __x
+///    The value from which bits are extracted.
+/// \param __y
+///    Specifies the index of the least significant bit at [13:8] 
+///    and the length at [5:0]; all other bits are ignored. 
+///    If bits [5:0] are zero, the length is interpreted as 64.
+///    If the sum of the index and length is greater than 64, the result is 
+///    undefined. If the length and index are both zero, bits [63:0] of 
+///    parameter __x are extracted. If the length is zero but the index is 
+///    non-zero, the result is undefined. 
+/// \returns A 128-bit vector whose lower 64 bits contain the bits extracted 
+///    from the source operand.
 static __inline__ __m128i __attribute__((__always_inline__, __nodebug__))
 _mm_extract_si64(__m128i __x, __m128i __y)
 {

   return (__m128i)__builtin_ia32_extrq((__v2di)__x, (__v16qi)__y);
 }
 
+/// \brief Inserts bits of a specified length from the source integer vector 
+///    y into the lower 64 bits of the destination integer vector x at the 
+///    index idx and of the length len.
+///
+/// \headerfile <x86intrin.h>
+///
+/// \code 
+/// __m128i _mm_inserti_si64(__m128i x, __m128i y, const int len,
+/// const int idx);
+/// \endcode 
+///
+/// \code 
+/// This intrinsic corresponds to the \c INSERTQ instruction.
+/// \endcode 
+///
+/// \param x
+///    The destination operand where bits will be inserted. The inserted bits 
+///    are defined by the length len and by the index idx specifying the least 
+///    significant bit.
+/// \param y
+///    The source operand containing the bits to be extracted. The extracted 
+///    bits are the least significant bits of operand y of length len.
+/// \param len
+///    Bits [5:0] specify the length; the other bits are ignored. If bits [5:0]
+///    are zero, the length is interpreted as 64.
+/// \param idx
+///    Bits [5:0] specify the index of the least significant bit; the other 
+///    bits are ignored. If the sum of the index and length is greater than 
+///    64, the result is undefined. If the length and index are both zero, 
+///    bits [63:0] of parameter y are inserted into parameter x. If the 
+///    length is zero but the index is non-zero, the result is undefined.
+/// \returns A 128-bit integer vector containing the original lower 64-bits 
+///    of destination operand x with the specified bitfields replaced by the
+///    lower bits of source operand y. The upper 64 bits of the return value 
+///    are undefined.
+
 #define _mm_inserti_si64(x, y, len, idx) \
   ((__m128i)__builtin_ia32_insertqi((__v2di)(__m128i)(x), \
                                     (__v2di)(__m128i)(y), \
                                     (char)(len), (char)(idx)))
 
+/// \brief Inserts bits of a specified length from the source integer vector 
+///    __y into the lower 64 bits of the destination integer vector __x at 
+///    the index and of the length specified by __y.
+///
+/// \headerfile <x86intrin.h>
+///
+/// \code 
+/// This intrinsic corresponds to the \c INSERTQ instruction.
+/// \endcode 
+///
+/// \param __x
+///    The destination operand where bits will be inserted. The inserted bits 
+///    are defined by the length and by the index of the least significant bit 
+///    specified by operand __y.
+/// \param __y
+///    The source operand containing the bits to be extracted. The extracted 
+///    bits are the least significant bits of operand __y with length specified
+///    by bits [69:64]. These are inserted into the destination at the index 
+///    specified by bits [77:72]; all other bits are ignored.
+///    If bits [69:64] are zero, the length is interpreted as 64.
+///    If the sum of the index and length is greater than 64, the result is 
+///    undefined. If the length and index are both zero, bits [63:0] of 
+///    parameter __y are inserted into parameter __x. If the length
+///    is zero but the index is non-zero, the result is undefined. 
+/// \returns A 128-bit integer vector containing the original lower 64-bits 
+///    of destination operand __x with the specified bitfields replaced by the
+///    lower bits of source operand __y. The upper 64 bits of the return value 
+///    are undefined.
+
 static __inline__ __m128i __attribute__((__always_inline__, __nodebug__))
 _mm_insert_si64(__m128i __x, __m128i __y)
 {

   return (__m128i)__builtin_ia32_insertq((__v2di)__x, (__v2di)__y);
 }
 
+/// \brief Stores a 64-bit double-precision value in a 64-bit memory location. 
+///    To minimize caching, the data is flagged as non-temporal (unlikely to be
+///    used again soon).
+///
+/// \headerfile <x86intrin.h>
+///
+/// \code 
+/// This intrinsic corresponds to the \c MOVNTSD instruction.
+/// \endcode 
+///
+/// \param __p
+///    The 64-bit memory location used to store the register value.
+/// \param __a
+///    The 64-bit double-precision floating-point register value to
+///    be stored.
 static __inline__ void __attribute__((__always_inline__, __nodebug__))
 _mm_stream_sd(double *__p, __m128d __a)
 {

   __builtin_ia32_movntsd(__p, (__v2df)__a);
 }
 
+/// \brief Stores a 32-bit single-precision floating-point value in a 32-bit
+///    memory location. To minimize caching, the data is flagged as
+///    non-temporal (unlikely to be used again soon).
+///
+/// \headerfile <x86intrin.h>
+///
+/// \code 
+/// This intrinsic corresponds to the \c MOVNTSS instruction.
+/// \endcode 
+///
+/// \param __p
+///    The 32-bit memory location used to store the register value.
+/// \param __a
+///    The 32-bit single-precision floating-point register value to
+///    be stored.
 static __inline__ void __attribute__((__always_inline__, __nodebug__))
 _mm_stream_ss(float *__p, __m128 __a)
 {