Page MenuHomePhabricator

avxintrin.h documentation fixes and updates
ClosedPublic

Authored by dyung on Dec 21 2017, 11:32 AM.

Details

Summary

This is the result of several patches we made internally to update the documentation that we would like to have reviewed for possible submission.

The changes include:

  1. Fix incorrect wording in various intrinsic descriptions. Previously the descriptions used "low-order" and "high-order" when the intended meaning was "even-indexed" and "odd-indexed".
  2. Fix a few typos and errors found during review.
  3. Restore new line endings and change to use hex suffixes (0h instead of 0x0).

These patches were made by Craig Flores

Diff Detail

Event Timeline

dyung created this revision.Dec 21 2017, 11:32 AM
lebedev.ri added inline comments.
lib/Headers/avxintrin.h
1668 ↗(On Diff #127915)

While i'm incompetent to actually review this, i have a question.
Why replace code-friendly 0x00 with 00h?
And, why is this hex in the first place? Why not decimals?

kromanova added inline comments.
lib/Headers/avxintrin.h
1668 ↗(On Diff #127915)

why is this hex in the first place? Why not decimals?

There are a few reasons that I could think of:

(1) for consistency with the defines in the header file describing these values:
#define _CMP_EQ_OQ 0x00 /* Equal (ordered, non-signaling) */
#define _CMP_LT_OS 0x01 /* Less-than (ordered, signaling) */
#define _CMP_LE_OS 0x02 /* Less-than-or-equal (ordered, signaling) */
#define _CMP_UNORD_Q 0x03 /* Unordered (non-signaling) */
#define _CMP_NEQ_UQ 0x04 /* Not-equal (unordered, non-signaling) */

(2) This immediate is 5 bits, so when using hex, it's much easier to tell which bits are set/not set/
(3) For consistency with Intel's and AMD's documentation for intrinsics and corresponding instructions.

I s developers prefer to have it in 0x format, this change it was done for consistency.
AMD and Intel manuals use the 'h' suffix style

1668 ↗(On Diff #127915)

Why replace code-friendly 0x00 with 00h?

For consistency with some other intrinsics doxygen documentation using 'h' notation.

I looked little further and noticed that around 60% of the doxygen comments are using '0x' notation, while around 40% are using 'h' notation. It's inconsistent. Now we could pick one or another. If someone has a strong opinion, please speak up. I personally prefer '0x' notation.

Doug, could you please change your code review to introduce '0x' notation back. I will submit a separate code review a little later to change the rest of "h" occurrences to "0x" in the rest of intrinsics documentation.

2378 ↗(On Diff #127915)

Please fit tightly into 80 chars.

dyung updated this revision to Diff 128861.Jan 6 2018, 5:33 PM

Updated change to address review feedback.

RKSimon accepted this revision.Jan 7 2018, 2:38 AM

LGTM

This revision is now accepted and ready to land.Jan 7 2018, 2:38 AM
kromanova accepted this revision.Jan 8 2018, 12:25 PM

LGTM too.