Page MenuHomePhabricator

mmintrin.h documentation fixes and updates
ClosedPublic

Authored by dyung on Dec 21 2017, 2:23 PM.

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

These patches were made by Craig Flores

Event Timeline

dyung created this revision.Dec 21 2017, 2:23 PM
craig.topper added inline comments.Dec 22 2017, 7:00 PM
lib/Headers/mmintrin.h
88 ↗(On Diff #137333)

Shouldn't this be MOVQ?

104 ↗(On Diff #137333)

Shouldn't this be MOVQ?

1292 ↗(On Diff #137333)

PXOR?

1384–1385 ↗(On Diff #137333)

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.

1403–1404 ↗(On Diff #137333)

This is overly specific

1421–1422 ↗(On Diff #137333)

This is overly specific

kromanova added inline comments.Jan 3 2018, 5:30 PM
lib/Headers/mmintrin.h
1292 ↗(On Diff #137333)

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–1385 ↗(On Diff #137333)

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 added inline comments.Jan 4 2018, 12:18 PM
lib/Headers/mmintrin.h
1292 ↗(On Diff #137333)

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–1385 ↗(On Diff #137333)

Agreed.

kromanova added inline comments.Jan 4 2018, 6:43 PM
lib/Headers/mmintrin.h
55 ↗(On Diff #137333)

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 #137333)

Same as above.

88 ↗(On Diff #137333)

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

104 ↗(On Diff #137333)

Same as above.

craig.topper added inline comments.Jan 4 2018, 7:19 PM
lib/Headers/mmintrin.h
55 ↗(On Diff #137333)

We can change it back to VMOVD/MOVD

dyung added inline comments.Jan 8 2018, 4:28 PM
lib/Headers/mmintrin.h
1403–1404 ↗(On Diff #137333)

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 added inline comments.Jan 8 2018, 4:30 PM
lib/Headers/mmintrin.h
1403–1404 ↗(On Diff #137333)

Yeah. I just shortened my comment from _mm_set1_pi32.

dyung updated this revision to Diff 129015.Jan 8 2018, 5:17 PM

Updating diff based on review feedback.

dyung marked 8 inline comments as done.Jan 8 2018, 5:18 PM
efriedma added inline comments.
lib/Headers/mmintrin.h
55 ↗(On Diff #137333)

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 added inline comments.Jan 15 2018, 4:40 AM
lib/Headers/mmintrin.h
55 ↗(On Diff #137333)

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

efriedma added inline comments.Jan 15 2018, 1:21 PM
lib/Headers/mmintrin.h
55 ↗(On Diff #137333)

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 this revision to Diff 136221.Feb 27 2018, 7:56 PM

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

dyung marked an inline comment as done.Feb 27 2018, 7:59 PM
dyung added inline comments.
lib/Headers/mmintrin.h
55 ↗(On Diff #137333)

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 added inline comments.Feb 28 2018, 5:22 AM
lib/Headers/mmintrin.h
1292 ↗(On Diff #137333)

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

dyung updated this revision to Diff 137333.Mar 7 2018, 1:04 AM
dyung marked an inline comment as done.

Update based on feedback from Simon.

dyung marked an inline comment as done.Mar 7 2018, 1:05 AM

Any more comments?

RKSimon accepted this revision.Mar 8 2018, 12:19 PM

LGTM - thanks

This revision is now accepted and ready to land.Mar 8 2018, 12:19 PM
This revision was automatically updated to reflect the committed changes.