Page MenuHomePhabricator

[X86] Enable intrinsics that convert float and bf16 data to each other
ClosedPublic

Authored by skan on May 23 2019, 6:16 PM.

Details

Summary

Scalar version :
_mm_cvtsbh_ss , _mm_cvtness_sbh

Vector version:
_mm512_cvtpbh_ps , _mm256_cvtpbh_ps
_mm512_maskz_cvtpbh_ps , _mm256_maskz_cvtpbh_ps
_mm512_mask_cvtpbh_ps , _mm256_mask_cvtpbh_ps

Diff Detail

Repository
rL LLVM

Event Timeline

skan created this revision.May 23 2019, 6:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2019, 6:16 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
craig.topper retitled this revision from Enable intrinsics that convert float and bf16 data to each other to [X86] Enable intrinsics that convert float and bf16 data to each other.May 23 2019, 10:52 PM
RKSimon added inline comments.May 26 2019, 8:20 AM
lib/Headers/avx512bf16intrin.h
52 ↗(On Diff #201107)

\headerfile <x86intrin.h>

54 ↗(On Diff #201107)

Please can you write a better description?

66 ↗(On Diff #201107)

\headerfile <x86intrin.h>

98 ↗(On Diff #201107)

style/clang-format changes like these should be separated into their own patch

skan updated this revision to Diff 201601.May 27 2019, 6:08 PM

rm clang-format fix in this patch

skan marked 3 inline comments as done.May 27 2019, 6:11 PM
skan added inline comments.
lib/Headers/avx512bf16intrin.h
52 ↗(On Diff #201107)

fixed in updated patch

66 ↗(On Diff #201107)

fixed in updated patch

98 ↗(On Diff #201107)

style changes are removed in the updated patch

craig.topper added inline comments.May 27 2019, 7:13 PM
lib/Headers/avx512bf16intrin.h
37 ↗(On Diff #201601)

This needs to be a comment that's useful to user's of the compiler not compiler developers. It can't reference an implementation function inside the compiler

skan marked an inline comment as done.May 27 2019, 7:23 PM
skan added inline comments.
lib/Headers/avx512bf16intrin.h
37 ↗(On Diff #201601)

i can not think out a comment that 's needed by users since other comments has explained this function clearly, can i just remove this comment?

craig.topper added inline comments.May 27 2019, 8:37 PM
lib/Headers/avx512bf16intrin.h
37 ↗(On Diff #201601)

Just write "This intrinsic does not correspond to a specific instruction,"

skan updated this revision to Diff 201605.May 27 2019, 9:00 PM

make comments for _mm_cvtsbh_ss and _mm_cvtness_sbh better

This revision is now accepted and ready to land.Jun 4 2019, 11:10 AM
RKSimon accepted this revision.Jun 4 2019, 11:22 AM

Yup, doxygen LGTM - cheers

lib/Headers/avx512bf16intrin.h
33 ↗(On Diff #201605)

Single Float Data

pengfei added a subscriber: pengfei.Jun 4 2019, 6:09 PM
skan updated this revision to Diff 203170.Jun 5 2019, 9:12 AM

When target feature avx512bf16 is enabled, avx512vl is not enabled implicity for some reasons by now. But _mm_cvtness_sbh, _mm256_cvtpbh_ps , _mm256_maskz_cvtpbh_ps , _mm256_mask_cvtpbh_ps need avx512vl feature. So I move their definition from avx512bf16intrin to avx512vlbf16intrin.h, and move corresponding test cases from avx512bf16-builtins.c to avx512vlbf16-builtins.c.

And the test for _mm_cvtness_sbh is fixed in the new patch.

skan marked an inline comment as done.Jun 5 2019, 9:17 AM
skan updated this revision to Diff 203330.Jun 6 2019, 4:14 AM

avoid implicit type conversion for intrinsic parameter

skan updated this revision to Diff 203762.Jun 9 2019, 6:57 PM

change mask paramater' s name from __M to __U in order to be consistent with other intrinsics in the file

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2019, 6:16 PM