This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add reduce_*_ep[i|u]8/16 series intrinsics.
ClosedPublic

Authored by FreddyYe on Dec 22 2022, 1:19 AM.

Diff Detail

Event Timeline

FreddyYe created this revision.Dec 22 2022, 1:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2022, 1:19 AM
Herald added a subscriber: pengfei. · View Herald Transcript
FreddyYe requested review of this revision.Dec 22 2022, 1:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2022, 1:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
FreddyYe updated this revision to Diff 484759.Dec 22 2022, 1:23 AM

Add release note.

pengfei added inline comments.Dec 22 2022, 1:29 AM
clang/lib/Headers/avx512vlbwintrin.h
2806–2814

Do we need such comments?

pengfei added inline comments.Dec 22 2022, 1:35 AM
clang/test/CodeGen/X86/avx512vlbw-reduceIntrin.c
2

Should the name starts with avx512bwvl?
Or put them into avx512bwvl-intrinsics.ll?

clang/test/CodeGen/X86/avx512vlbw-reduceMinMaxIntrin.c
1 ↗(On Diff #484759)

Maybe not worth a new file?

pengfei added inline comments.Dec 22 2022, 1:37 AM
clang/test/CodeGen/X86/avx512vlbw-reduceIntrin.c
2

Sorry, I referred to the backend file. Please ignore.

FreddyYe updated this revision to Diff 484764.Dec 22 2022, 1:47 AM
FreddyYe marked 4 inline comments as done.

Address comments.

clang/test/CodeGen/X86/avx512vlbw-reduceMinMaxIntrin.c
1 ↗(On Diff #484759)

Agree

pengfei accepted this revision.Dec 22 2022, 1:50 AM

LGTM. Please wait some days for other reviewers.

This revision is now accepted and ready to land.Dec 22 2022, 1:50 AM
skan added inline comments.Dec 22 2022, 2:37 AM
clang/docs/ReleaseNotes.rst
806

This is not clear to me, try sth like "mm*_reduce_(add|and|max)_epi" and put the name in a code-block like the previous lines.

clang/test/CodeGen/X86/avx512vlbw-reduceIntrin.c
2

drop "-apple-darwin"?

116

char has different meaning on different platforms. Should we use "signed char" or "unsigned char" explicitly?

FreddyYe updated this revision to Diff 484784.Dec 22 2022, 3:01 AM
FreddyYe marked 3 inline comments as done.

Address comments.

skan added inline comments.Dec 22 2022, 5:18 PM
clang/lib/Headers/avx512vlbwintrin.h
2997

Should we update here too?

FreddyYe updated this revision to Diff 484998.Dec 22 2022, 5:40 PM
FreddyYe marked an inline comment as done.

Address comments.

skan added inline comments.Dec 22 2022, 5:46 PM
clang/test/CodeGen/X86/avx512vlbw-reduceIntrin.c
116

char has different meaning on different platforms. Should we use "signed char" or "unsigned char" explicitly?

Sorry, I think you misuderstood. It's a question, not a request. I already saw

_mm_mask_set1_epi8 (__m128i __O, __mmask16 __M, char __A)
_mm_maskz_set1_epi8 (__mmask16 __M, char __A)
_mm256_mask_set1_epi8 (__m256i __O, __mmask32 __M, char __A)
_mm256_maskz_set1_epi8 (__mmask32 __M, char __A)

in this file, but I am not if using "char" is appropriate.

FreddyYe updated this revision to Diff 485018.Dec 22 2022, 7:16 PM
FreddyYe marked an inline comment as done.

Address comments.

clang/test/CodeGen/X86/avx512vlbw-reduceIntrin.c
116

After investigating, I think we should use signed char and _v\d+qs instead of _v\d+qi for new intrinsic here for 2 reasons:

  1. _epi8 means signed i8 here.
  2. implement is using LLVM builtins but not x86 builtins.

And intrinsics not fitting two conditions above don't need to use explictly signed char.

skan accepted this revision.Dec 22 2022, 7:54 PM
skan added inline comments.
clang/test/CodeGen/X86/avx512vlbw-reduceIntrin.c
116

Make sense

This revision was landed with ongoing or failed builds.Dec 22 2022, 10:55 PM
This revision was automatically updated to reflect the committed changes.