mmintrin.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 instruction mappings/listings for various intrinsics

These patches were made by Craig Flores

[craig.topper]

lib/Headers/mmintrin.h

88 ↗

(On Diff #127946)

Shouldn't this be MOVQ?

104 ↗

(On Diff #127946)

Shouldn't this be MOVQ?

1292 ↗

(On Diff #127946)

PXOR?

1384 ↗

(On Diff #127946)

This is overly specific there is no guarantee we'd use those instructions. If it was a constant we'd probably just use a load.

1402 ↗

(On Diff #127946)

This is overly specific

1419 ↗

(On Diff #127946)

This is overly specific

[kromanova]

lib/Headers/mmintrin.h

1292 ↗

(On Diff #127946)

For which platform/compiler?

I checked, for x86_64 Linux XORPS(no avx)/VXORPS (with -mavx) is generated.
For PS4 we generate XORL.

I guess, we need to write something more generic, implying that an appropriate platform-specific XOR instruction is generated.

1384 ↗

(On Diff #127946)

That's right. I think we should use the following wording to match other _mm_set* intrinsics documentation in this file.

/ This intrinsic is a utility function and does not correspond to a specific
/ instruction.

[craig.topper]

lib/Headers/mmintrin.h

1292 ↗

(On Diff #127946)

Ideally to interoperate with other mmx intrinsics it should have been a PXOR into an mmx register. But apparently our mmx support is so limited that we aren't capable of that and instead create it in another domain and move it over.

I guess just indicate it as a utility function with no specific instruction.

1384 ↗

(On Diff #127946)

Agreed.

[kromanova]

lib/Headers/mmintrin.h

55 ↗

(On Diff #127946)

I tried clang on Linux, x86_64, and if -mavx option is passed, we generate VMOVD, if this option is omitted, we generate MOVD.
I think I understand the rational behind this change (namely, to keep MOVD, but remove VMOVD),
since this intrinsic should use MMX registers and shouldn't have corresponding AVX instruction(s).

However, that's what we generate at the moment when -mavx is passed (I suspect because our MMX support is limited)
vmovd %edi, %xmm0

Since we are writing the documentation for clang compiler, we should document what clang compiler is doing, not what is should be doing.
Craig, what do you think? Should we revert back to VMOVD/MOVD?

72 ↗

(On Diff #127946)

Same as above.

88 ↗

(On Diff #127946)

Yes, that's correct, (MOVQ) + the same question as above whether we should keep VMOVQ/MOVQ.

104 ↗

(On Diff #127946)

Same as above.

[craig.topper]

lib/Headers/mmintrin.h

55 ↗

(On Diff #127946)

We can change it back to VMOVD/MOVD

[dyung]

lib/Headers/mmintrin.h

1402 ↗

(On Diff #127946)

Just to be clear, when you say this is overly specific, you are saying that it should be replaced with the text "This intrinsic is a utility function and does not correspond to a specific instruction." Is that correct?

[craig.topper]

lib/Headers/mmintrin.h

1402 ↗

(On Diff #127946)

Yeah. I just shortened my comment from _mm_set1_pi32.

[dyung]

Updating diff based on review feedback.

[efriedma]

lib/Headers/mmintrin.h

55 ↗

(On Diff #127946)

The reference to vmovd seems confusing. Yes, LLVM compiles _mm_movpi64_epi64(_mm_cvtsi32_si64(i)) to vmovd, but that doesn't mean either of those intrinsics "corresponds" to vmovd; that's just the optimizer combining two operations into one.

[RKSimon]

lib/Headers/mmintrin.h

55 ↗

(On Diff #127946)

Should all these _mm_cvt* intrinsics be replaced with a 'this is a utility function' style comment like the _mm_set1* intrinsics?

[efriedma]

lib/Headers/mmintrin.h

55 ↗

(On Diff #127946)

In general, I think "corresponds to" should mean "if the inputs are produced by an inline asm, and the output is used by an inline asm, and the lowering will produce a single instruction, what instruction will we generate?". That's unambiguous, and will usually give a useful hint to the user. In this case, on trunk, the answer is consistently "movd".

Otherwise, it's not clear what it means for an intrinsic to correspond to anything; optimizations exist which can modify the instructions we choose for almost any intrinsic.

[dyung]

Updated documentation for instruction generated for _mm_cvtsi32_si64, _mm_cvtsi64_si32, _mm_cvtsi64_m64 and _mm_cvtm64_si64 based on feedback.

[dyung]

lib/Headers/mmintrin.h

55 ↗

(On Diff #127946)

Thanks for the feedback. I wrote a few small examples and noted that the instructions generated by the compiler were the non-avx form by default for this and the next three _mm_cvt* intrinsics as you indicated, so I have updated the documentation to list the non-avx instruction. Additionally, in some discussions, it was pointed out that these are MMX intrinsics, so the AVX instructions would not have been generated.

[RKSimon]

lib/Headers/mmintrin.h

1292 ↗

(On Diff #127946)

D41908 fixed MMX zero constant creation so you should be able to say "This intrinsic corresponds to the <c> PXOR </c> instruction."

[dyung]

Update based on feedback from Simon.

[RKSimon]

Any more comments?

[craig.topper]

I'm happy.

[RKSimon]

LGTM - thanks

cfe/trunk/lib/Headers/mmintrin.h

_mm_empty(void)
     __builtin_ia32_emms();
 }
 
 /// \brief Constructs a 64-bit integer vector, setting the lower 32 bits to the
 ///    value of the 32-bit integer parameter and setting the upper 32 bits to 0.
 ///
 /// \headerfile <x86intrin.h>
 ///
-/// This intrinsic corresponds to the <c> VMOVD / MOVD </c> instruction.
+/// This intrinsic corresponds to the <c> MOVD </c> instruction.
 ///
 /// \param __i
 ///    A 32-bit integer value.
 /// \returns A 64-bit integer vector. The lower 32 bits contain the value of the
 ///    parameter. The upper 32 bits are set to 0.
 static __inline__ __m64 __DEFAULT_FN_ATTRS
 _mm_cvtsi32_si64(int __i)
 {
     return (__m64)__builtin_ia32_vec_init_v2si(__i, 0);
 }
 
 /// \brief Returns the lower 32 bits of a 64-bit integer vector as a 32-bit
 ///    signed integer.
 ///
 /// \headerfile <x86intrin.h>
 ///
-/// This intrinsic corresponds to the <c> VMOVD / MOVD </c> instruction.
+/// This intrinsic corresponds to the <c> MOVD </c> instruction.
 ///
 /// \param __m
 ///    A 64-bit integer vector.
 /// \returns A 32-bit signed integer value containing the lower 32 bits of the
 ///    parameter.
 static __inline__ int __DEFAULT_FN_ATTRS
 _mm_cvtsi64_si32(__m64 __m)
 {
     return __builtin_ia32_vec_ext_v2si((__v2si)__m, 0);
 }
 
 /// \brief Casts a 64-bit signed integer value into a 64-bit integer vector.
 ///
 /// \headerfile <x86intrin.h>
 ///
-/// This intrinsic corresponds to the <c> VMOVQ / MOVD </c> instruction.
+/// This intrinsic corresponds to the <c> MOVQ </c> instruction.
 ///
 /// \param __i
 ///    A 64-bit signed integer.
 /// \returns A 64-bit integer vector containing the same bitwise pattern as the
 ///    parameter.
 static __inline__ __m64 __DEFAULT_FN_ATTRS
 _mm_cvtsi64_m64(long long __i)
 {
     return (__m64)__i;
 }
 
 /// \brief Casts a 64-bit integer vector into a 64-bit signed integer value.
 ///
 /// \headerfile <x86intrin.h>
 ///
-/// This intrinsic corresponds to the <c> VMOVQ / MOVD </c> instruction.
+/// This intrinsic corresponds to the <c> MOVQ </c> instruction.
 ///
 /// \param __m
 ///    A 64-bit integer vector.
 /// \returns A 64-bit signed integer containing the same bitwise pattern as the
 ///    parameter.
 static __inline__ long long __DEFAULT_FN_ATTRS
 _mm_cvtm64_si64(__m64 __m)
 {

 {
     return (__m64)__builtin_ia32_pcmpgtd((__v2si)__m1, (__v2si)__m2);
 }
 
 /// \brief Constructs a 64-bit integer vector initialized to zero.
 ///
 /// \headerfile <x86intrin.h>
 ///
-/// This intrinsic corresponds to the <c> VXORPS / XORPS </c> instruction.
+/// This intrinsic corresponds to the <c> PXOR </c> instruction.
 ///
 /// \returns An initialized 64-bit integer vector with all elements set to zero.
 static __inline__ __m64 __DEFAULT_FN_ATTRS
 _mm_setzero_si64(void)
 {
     return (__m64){ 0LL };
 }
 

 }
 
 /// \brief Constructs a 64-bit integer vector of [2 x i32], with each of the
 ///    32-bit integer vector elements set to the specified 32-bit integer
 ///    value.
 ///
 /// \headerfile <x86intrin.h>
 ///
-/// This intrinsic corresponds to the <c> VPSHUFD / PSHUFD </c> instruction.
+/// This intrinsic is a utility function and does not correspond to a specific
+///    instruction.
 ///
 /// \param __i
 ///    A 32-bit integer value used to initialize each vector element of the
 ///    result.
 /// \returns An initialized 64-bit integer vector of [2 x i32].
 static __inline__ __m64 __DEFAULT_FN_ATTRS
 _mm_set1_pi32(int __i)
 {
     return _mm_set_pi32(__i, __i);
 }
 
 /// \brief Constructs a 64-bit integer vector of [4 x i16], with each of the
 ///    16-bit integer vector elements set to the specified 16-bit integer
 ///    value.
 ///
 /// \headerfile <x86intrin.h>
 ///
-/// This intrinsic corresponds to the <c> VPSHUFLW / PSHUFLW </c> instruction.
+/// This intrinsic is a utility function and does not correspond to a specific
+///    instruction.
 ///
 /// \param __w
 ///    A 16-bit integer value used to initialize each vector element of the
 ///    result.
 /// \returns An initialized 64-bit integer vector of [4 x i16].
 static __inline__ __m64 __DEFAULT_FN_ATTRS
 _mm_set1_pi16(short __w)
 {
     return _mm_set_pi16(__w, __w, __w, __w);
 }
 
 /// \brief Constructs a 64-bit integer vector of [8 x i8], with each of the
 ///    8-bit integer vector elements set to the specified 8-bit integer value.
 ///
 /// \headerfile <x86intrin.h>
 ///
-/// This intrinsic corresponds to the <c> VPUNPCKLBW + VPSHUFLW / PUNPCKLBW +
-///    PSHUFLW </c> instruction.
+/// This intrinsic is a utility function and does not correspond to a specific
+///    instruction.
 ///
 /// \param __b
 ///    An 8-bit integer value used to initialize each vector element of the
 ///    result.
 /// \returns An initialized 64-bit integer vector of [8 x i8].
 static __inline__ __m64 __DEFAULT_FN_ATTRS
 _mm_set1_pi8(char __b)
 {