Adding doxygen comments to the LLVM intrinsics (part 2, _wmmintrin_pclmul.h)

[kromanova]

Summary

Hello,

Here is the patch with the doxygen comments for the intrinsincs in the header file __wmmintrin_pclmul.h.
The doxygen comments are automatically generated based on our internal intrinsics document.

I will submit more doxygen comments for the other intrinsic header files as soon as this patch is approved.

Here is the link to the general discussion about adding comments to x86 intrinsics headers.
http://permalink.gmane.org/gmane.comp.compilers.clang.devel/42032

Here is the link to the similar code review for the doxygen comments for the header file ammintrin.h "Adding doxygen comments to the LLVM intrinsics (part 1, ammintrin.h)"
http://reviews.llvm.org/D8762

Please review.

Katya.

[silvas]

This may sound stupid, but: can you benchmark the time it takes to build some project (that actually uses intrinsics in most translation units, e.g. a game) with the headers w/ and w/o the doxygen comments to check that all the extra comment skipping doesn't affect compilation time? I.e. run your script to add the comments for "all" the intrinsic headers (similar to what you expect the final state to be after all these patches) and test the build time of a game (and compare with the unmodified headers).

Also, can you post a patch that changes "all" the headers to have doxygen comments like you intend, so that others can test and verify?

[kromanova]

In D15999#330794, @silvas wrote:

This may sound stupid, but: can you benchmark the time it takes to build some project (that actually uses intrinsics in most translation units, e.g. a game) with the headers w/ and w/o the doxygen comments to check that all the extra comment skipping doesn't affect compilation time? I.e. run your script to add the comments for "all" the intrinsic headers (similar to what you expect the final state to be after all these patches) and test the build time of a game (and compare with the unmodified headers).

This makes sense. I can do this, though it might take a couple of days, since I don't have a setup for a game build.
I suspect that that we shouldn't use an application/game that is using precompiled headers. Any other limitation should be imposed on the beanchmark?

Also, can you post a patch that changes "all" the headers to have doxygen comments like you intend, so that others can test and verify?

Yes, I'll do this. However, there are a couple of things that the people should be aware:
(a) *only* intrinsics that are supported on PS4 will have doxygen comments. PS4 documentation is parsed to generate these comments, and it doesn't document the intrinsics that are not supported on PS4.
(b) the doxygen comments for the rest (not submitted upstream) headers files might have some problems (typos, incorrect parameter types, bugs in the description, etc). The tool that generates the comments might have some glitches too. Most of this things get fixed after the manual review that I do before I submit the Phabricator code review. So, please don't assume that this is the final version of the header files with doxygen comments.

[kromanova]

In D15999#330794, @silvas wrote:

This may sound stupid, but: can you benchmark the time it takes to build some project (that actually uses intrinsics in most translation units, e.g. a game) with the headers w/ and w/o the doxygen comments to check that all the extra comment skipping doesn't affect compilation time? I.e. run your script to add the comments for "all" the intrinsic headers (similar to what you expect the final state to be after all these patches) and test the build time of a game (and compare with the unmodified headers).

Also, can you post a patch that changes "all" the headers to have doxygen comments like you intend, so that others can test and verify?

Out of curiosity, do you know if the impact to the build time for Eric's change of starting to use the target attributes instead of conditional inclusion was measured (r239883) on a large scale application. If so, what were the results? I should probably include Eric.

[silvas]

In D15999#331601, @kromanova wrote:

In D15999#330794, @silvas wrote:

This may sound stupid, but: can you benchmark the time it takes to build some project (that actually uses intrinsics in most translation units, e.g. a game) with the headers w/ and w/o the doxygen comments to check that all the extra comment skipping doesn't affect compilation time? I.e. run your script to add the comments for "all" the intrinsic headers (similar to what you expect the final state to be after all these patches) and test the build time of a game (and compare with the unmodified headers).

Also, can you post a patch that changes "all" the headers to have doxygen comments like you intend, so that others can test and verify?

Out of curiosity, do you know if the impact to the build time for Eric's change of starting to use the target attributes instead of conditional inclusion was measured (r239883) on a large scale application. If so, what were the results? I should probably include Eric.

I don't think we did any testing at SCE, but we probably should have. I don't think Google's primary codebases (nor Apple's, or anybody else pretty much) include the intrinsic headers in basically every TU so the impact is probably not a huge concern overall. However, games include these headers in basically every TU so we at SCE should take a look.

Also, the target attributes stuff had some correctness issues (e.g. crashes in backend) that took a while to iron out and those issues took the limelight.

[silvas]

In D15999#331512, @kromanova wrote:

In D15999#330794, @silvas wrote:

This may sound stupid, but: can you benchmark the time it takes to build some project (that actually uses intrinsics in most translation units, e.g. a game) with the headers w/ and w/o the doxygen comments to check that all the extra comment skipping doesn't affect compilation time? I.e. run your script to add the comments for "all" the intrinsic headers (similar to what you expect the final state to be after all these patches) and test the build time of a game (and compare with the unmodified headers).

This makes sense. I can do this, though it might take a couple of days, since I don't have a setup for a game build.
I suspect that that we shouldn't use an application/game that is using precompiled headers. Any other limitation should be imposed on the beanchmark?

Just make sure it is realistic.

Also, can you post a patch that changes "all" the headers to have doxygen comments like you intend, so that others can test and verify?

Yes, I'll do this. However, there are a couple of things that the people should be aware:
(a) *only* intrinsics that are supported on PS4 will have doxygen comments. PS4 documentation is parsed to generate these comments, and it doesn't document the intrinsics that are not supported on PS4.

If we don't have coverage of every intrinsic, we will need to add documentation for Clang developers about the comment format so that future intrinsics (and intrinsics not covered) can be documented.

(b) the doxygen comments for the rest (not submitted upstream) headers files might have some problems (typos, incorrect parameter types, bugs in the description, etc). The tool that generates the comments might have some glitches too. Most of this things get fixed after the manual review that I do before I submit the Phabricator code review. So, please don't assume that this is the final version of the header files with doxygen comments.

Sure. Makes sense.

[kromanova]

I see. I was hoping that if you were, I could have used the same benchmark and compare what build time increase caused by using target attributes (might be much more substantial) and by adding doxygen comments.

From: Eric Christopher [mailto:[e-mail]]
Sent: Wednesday, January 20, 2016 3:09 PM
To: [e-mail]; Romanova, Katya; [e-mail]; [e-mail]; Gao, Yunzhong; Sean Silva
Cc: [e-mail]
Subject: Re: [PATCH] D15999: Adding doxygen comments to the LLVM intrinsics (part 2, _wmmintrin_pclmul.h)

[echristo]

I have some benchmarks, but none of them moved when I added the support
that I could see. It wouldn't catch small regressions though.

-eric

[kromanova]

In D15999#331639, @silvas wrote:

In D15999#331601, @kromanova wrote:

I don't think we did any testing at SCE, but we probably should have. I don't think Google's primary codebases (nor Apple's, or anybody else pretty much) include the intrinsic headers in basically every TU so the impact is probably not a huge concern overall. However, games include these headers in basically every TU so we at SCE should take a look.

Yes, typical PS4 codebase will heavily include x86 intrinsics headers. At the same time, typical PS4 codebase will use precompiled headers or modules for most of the time, so I hope the real world impact of doxygen comment will not be huge in any case. BTW, in my experiment, I'm planning to measure worst case scenario (no modules, no PCH). I think t(ahe benefit of having (1) tooltips and autocomplication in XCode, (2) Intellisense tooltips in MS Visual Studio, (3) internal intrinsics documentation for clang, (4) ability to generate up-to-date internal intrinsic documentation based on the latest Clang headers, etc, should overweigh the drawback of slower compile time for the applications/games that are heavily using intrinsic headers, but opted not to use modules or PCH.
Still, I agree that we need to make this measurement. And I'm curious too :)

[kromanova]

In D15999#331649, @silvas wrote:

In D15999#331512, @kromanova wrote:

In D15999#330794, @silvas wrote:

Also, can you post a patch that changes "all" the headers to have doxygen comments like you intend, so that others can test and verify?

Yes, I'll do this. However, there are a couple of things that the people should be aware:
(a) *only* intrinsics that are supported on PS4 will have doxygen comments. PS4 documentation is parsed to generate these comments, and it doesn't document the intrinsics that are not supported on PS4.

If we don't have coverage of every intrinsic, we will need to add documentation for Clang developers about the comment format so that future intrinsics (and intrinsics not covered) can be documented.

That was the plan... The consistency of the doxygen documentation will be especially important for the individuals/companies that decide to generate their up-to-date intrinsics guide based on the latest Clang headers. One doesn't want to see differently formatted intrinsics guide.

(b) the doxygen comments for the rest (not submitted upstream) headers files might have some problems (typos, incorrect parameter types, bugs in the description, etc). The tool that generates the comments might have some glitches too. Most of this things get fixed after the manual review that I do before I submit the Phabricator code review. So, please don't assume that this is the final version of the header files with doxygen comments.

Sure. Makes sense.

[kromanova]

In this code review, Sean Silva asked me to post a patch with "all" the headers with doxygen comments, so that others can test and verify.
Here is the patch automatically generated by DCG tool (doxygen comment generator) that I wrote.

There are a couple of things that the people should be aware:
(a) *only* intrinsics that are supported on PS4 will have doxygen comments. PS4 documentation is parsed to generate the comments and it doesn't document the intrinsics that are *not* supported on PS4.
(b) the doxygen comments for all the headers (except ammintrin.h that was already accepted upstream and _wmmintrin_pclmul.h that is currently in the review) might have some known problems (typos, incorrect parameter types, bugs in the description, etc). DCG might have some glitches too (e.g. currently it doesn't parse HTML tables correctly). Most of these things will get fixed before I submit the Phabricator code review for a specific file.

So, please don't assume that this is the final version of the headers with doxygen comments. It's a rough dratf. Use with caution! :)
However, it should be perfectly acceptable for measuring build time performance impact.

[silvas]

For the preview of all the changes, can you please put that in a separate patch from this one?

[kromanova]

In D15999#333173, @silvas wrote:

For the preview of all the changes, can you please put that in a separate patch from this one?

Done.
See http://reviews.llvm.org/D16442

[kromanova]

I did some build time measurement on a big game code where the the intrisics are heavily used (PHC was enabled). The presence of comments didn't make any noticeable difference.
Sean did similar measurements without PCH and didn't see any difference in performance neither.

[kromanova]

SCE's techinical writer, Craig Flores, did the code review and suggested a few changes to the documentation.

[kromanova]

Hello,
I have a lot of intrinsic header files with doxygen documentation that I will be submitting in the near future.

We have been reviewing this internally at SCE to make sure the patch it in the best shape before submitting and make the reviews easier. Every patch has been already reviewed by at least three people (me, one of SCE's compiler developer and SCE's technical writer). So, hopefully the quality should be pretty good.

Still, there are a lot of intrinsics (~800) to review and I don’t want to put the burden on only 1-2 open source reviewers.

Who else (except people on the 'reviewers' and 'subscribers' list) will be a good reviewer for X86 intrinsics?
Are there any intrinsics experts from MS, Intel, AMD, Apple on the mailing list who might be willing to help out?”

Thanks!
Katya.

[echristo]

Honestly if they've been reviewed like that internally I'm ok with you just committing them - especially if they look like this.

The only concerns I'd have are in the case of "This intrinsic corresponds to the <blank> instruction" (side note, use the "the"? I commented on a case inline). This isn't always the case with all of our intrinsics when the compiler lowers them to a shuffle intrinsic or some such, or it's optimized, etc. Personally I'd leave that line out, though I understand it exists in a lot of similar documentation.

-eric

lib/Headers/__wmmintrin_pclmul.h

37 ↗

(On Diff #45902)

"corresponds to the" ?

[kromanova]

In D15999#335653, @echristo wrote:

Honestly if they've been reviewed like that internally I'm ok with you just committing them - especially if they look like this.

The only concerns I'd have are in the case of "This intrinsic corresponds to the <blank> instruction" (side note, use the "the"? I commented on a case inline). This isn't always the case with all of our intrinsics when the compiler lowers them to a shuffle intrinsic or some such, or it's optimized, etc. Personally I'd leave that line out, though I understand it exists in a lot of similar documentation.

Hi Eric,

I agree. Sometimes the instruction that corresponds to a specific intrinsic is optimized out, sometimes it will get lowered to something else, etc.

However, I think keeping the instruction name in the documentation is extremely useful. In general, intrinsic documentation (especially in the form of comments) is not very complete. When I need to know what a specific intrinsic is doing (and I very often have to look up intrinsics!), I find the corresponding instruction name and go dig in AMD's or Intel's Architecture Programmer's manuals, where I could find all the details I need. Programmer's manuals instruction descriptions are much more detailed and complete. However, it's too much information to add to the comments. :)

As you know, Intel's and MS's intrinsics guides are also specifying corresponding instruction names for the intrinsics. I suspect they had the same idea that I just described.

I briefly chatted with Paul Robinson and he suggested to say "This intrinsic is equivalent to the <blah> instruction" instead, because this sentence doesn't give a false impression that one will definitely see this particular instruction in the generated code. Intel's intrinsics documentation says something like that, e.g: "The corresponding Intel® AVX instruction is VBLENDPD"

What do you think/prefer?

And, yes, I will add "the" before "corresponds to". Thanks! Easy enough with the generator. :)

[kromanova]

In D15999#335653, @echristo wrote:

Honestly if they've been reviewed like that internally I'm ok with you just committing them - especially if they look like this.

The only concerns I'd have are in the case of "This intrinsic corresponds to the <blank> instruction" (side note, use the "the"? I commented on a case inline). This isn't always the
case with all of our intrinsics when the compiler lowers them to a shuffle intrinsic or some such, or it's optimized, etc. Personally I'd leave that line out, though I understand it exists > in a lot of similar documentation.

BTW, in some cases, our documentation won't be as specific and will say "This intrinsic (e.g. _mm_store_ps1 ) corresponds to the Shuffling + MOVSS instruction" or
"No AVX instruction corresponds to this intrinsic (e.g. _mm256_set_pd)" or "Composite SSE2 instruction corresponds to this intrinsic (e.g. _mm_set_sd).

Microsoft and Intel's documentation are very similar with this respect. See the description of _mm_set_sd intrinsic that I just mentioned.

https://software.intel.com/en-us/node/524261
https://msdn.microsoft.com/en-us/library/dksztbt9%28v=vs.90%29.aspx

[echristo]

Yep. As I said, I expect to be on the losing side of that argument - just too much prior art :)

-eric

cfe/trunk/lib/Headers/__wmmintrin_pclmul.h

  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  *
  *===-----------------------------------------------------------------------===
  */
 #ifndef _WMMINTRIN_PCLMUL_H
 #define _WMMINTRIN_PCLMUL_H
 
+/// \brief Multiplies two 64-bit integer values, which are selected from source
+///    operands using the immediate-value operand. The multiplication is a 
+///    carry-less multiplication, and the 128-bit integer product is stored in
+///    the destination.
+///
+/// \headerfile <x86intrin.h>
+///
+/// \code 
+/// __m128i _mm_clmulepi64_si128(__m128i __X, __m128i __Y, const int __I);
+/// \endcode 
+///
+/// This intrinsic corresponds to the \c VPCLMULQDQ instruction.
+///
+/// \param __X
+///    A 128-bit vector of [2 x i64] containing one of the source operands.
+/// \param __Y
+///    A 128-bit vector of [2 x i64] containing one of the source operands.
+/// \param __I
+///    An immediate value specifying which 64-bit values to select from the
+///    operands.
+///    Bit 0 is used to select a value from operand __X,
+///    and bit 4 is used to select a value from operand __Y:
+///    Bit[0]=0 indicates that bits[63:0] of operand __X are used.
+///    Bit[0]=1 indicates that bits[127:64] of operand __X are used.
+///    Bit[4]=0 indicates that bits[63:0] of operand __Y are used.
+///    Bit[4]=1 indicates that bits[127:64] of operand __Y are used.
+/// \returns The 128-bit integer vector containing the result of the carry-less
+///    multiplication of the selected 64-bit values.
 #define _mm_clmulepi64_si128(__X, __Y, __I) \
   ((__m128i)__builtin_ia32_pclmulqdq128((__v2di)(__m128i)(__X), \
                                         (__v2di)(__m128i)(__Y), (char)(__I)))
 
 #endif /* _WMMINTRIN_PCLMUL_H */