xmmintrin.h documentation fixes and updates

[dyung]

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 inaccurate instruction listings.
2. Fix small issues in _mm_getcsr and _mm_setcsr.
3. Fix description of NaN handling in comparison intrinsics.
4. Fix inaccurate description of _mm_movemask_pi8.
5. Fix inaccurate instruction mappings.
6. Fix typos.
7. Clarify wording on some descriptions.
8. Fix bit ranges in return value.
9. Fix typo in _mm_move_ms intrinsic instruction since it operates on singe-precision values, not double.

These patches were made by Craig Flores

[craig.topper]

lib/Headers/xmmintrin.h

1706

There is no BROADCASTSS instruction. That's an AVX instruction that only exists as VBROADCASTSS. The orginal comment was correct for pre-AVX.

2202

Why is VPINSRW removed?

2662–2663

MOVSS is correct for pre SSE4.1 targets.

[kromanova]

lib/Headers/xmmintrin.h

2202

I suspect the rational is the same I talked about in mmintrin.h review.
This intrinsic should use MMX registers and shouldn't have corresponding AVX instruction(s).

I've tried this and with or without -mavx for Linux/x86_64 we generate PINSRW in both cases (i.e. I wasn't able to trigger generation of VEX prefixed instruction).

m64 foo (m64 a, int b)
{

__m64 x;
x = _mm_insert_pi16 (a, b, 0);
return x;

}

2662–2663

That's correct.
Doug, I think we should write:
VBLENDPS / BLENDPS / MOVSS

[craig.topper]

lib/Headers/xmmintrin.h

2202

I didn't realize this was a MMX type intrinsic in the xmmintrin file. So VPINSRW being removed makes sense. Sorry for the noise.

[RKSimon]

Sort of related - should we enable -Wdocumentation (it's currently -Wall and -Weverything might be too much....) on the respective clang builtin tests? Doesn't have to be part of this patch.

lib/Headers/xmmintrin.h

1927

Not necessarily, it could be (v)movhps/(v)movhpd - but those are tricky to generate in current isel.

[kromanova]

In D41523#968359, @RKSimon wrote:

Sort of related - should we enable -Wdocumentation (it's currently -Wall and -Weverything might be too much....) on the respective clang builtin tests? Doesn't have to be part of this patch.

Good idea, Simon. I will give it a shot. Could you please let me know the name of the directory, where clang builtin tests are located?

[craig.topper]

The builtins are tested in tests like test/CodeGen/sse-builtins.c

[dyung]

lib/Headers/xmmintrin.h

1927

Would it be preferred then for this to be the following:

/// This intrinsic corresponds to the <c> VPEXTRQ / PEXTRQ / VMOVHPS / VMOVHPD / MOVHPS / MOVHPD </c> instruction.

[dyung]

Update review based on feedback.

[kromanova]

In D41523#968776, @craig.topper wrote:

The builtins are tested in tests like test/CodeGen/sse-builtins.c,

Thank you!

I wonder if -Wdocumentation is working...
I have enabled it for a few tests, like avx-builtins.c, sse-builtins.c and re-run the tests; everything was fine, no errors.

I was suspicious that -Wdocumentation might not be catching the errors and intentionally broke a few doxygen comments in avxintrin.h header for _mm256_add_pd and _mm256_add_ps (these intrinsics were the first ones used in avx-builtins.c) by mismatching the parameter names in the doxygen comments and in definitions, by removing doxygen comments section describing the parameter names and eventually by removing the entire doxygen comment for these intrinsics. However, -Wdocumentation -Werror haven't reported any errors.

Am I missing something? What kind of "documentation" problems -Wocumentation option catches?

[RKSimon]

Apart for the -Wdocumentation issue (which can be handled separately), is there anything else stalling this ticket?

[craig.topper]

LGTM