Page MenuHomePhabricator

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

Authored by kromanova on Apr 1 2015, 1:47 AM.

Details

Reviewers
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.

Diff Detail

Event Timeline

kromanova updated this revision to Diff 23033.Apr 1 2015, 1:47 AM
kromanova retitled this revision from to Adding doxygen comments to the LLVM intrinsics (part 1, ammintrin.h).
kromanova updated this object.
kromanova edited the test plan for this revision. (Show Details)
kromanova added a subscriber: Unknown Object (MLST).
ygao added a subscriber: ygao.Apr 1 2015, 1:22 PM
kromanova updated this object.Apr 9 2015, 7:08 PM

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 added inline comments.
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 updated this revision to Diff 26303.May 21 2015, 7:54 PM

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.

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?

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.

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!

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?

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.

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.

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.

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

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

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

That's what being displayed with \remark

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.

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.

For avoidance of doubt: LGTM too.

kromanova accepted this revision.Dec 9 2015, 7:41 PM
kromanova added a reviewer: kromanova.

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

This revision is now accepted and ready to land.Dec 9 2015, 7:41 PM
kromanova closed this revision.Dec 9 2015, 7:42 PM