This is an archive of the discontinued LLVM Phabricator instance.

[X86][intrinsics] lower _mm[256|512]_mask[z]_set1_epi[8|16|32|64] intrinsic to IR
ClosedPublic

Authored by jina.nahias on Sep 10 2017, 4:31 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

jina.nahias created this revision.Sep 10 2017, 4:31 AM
RKSimon edited edge metadata.Sep 10 2017, 4:40 AM

As with D37562, strip the builtins from include/clang/Basic/BuiltinsX86.def

delete from include/clang/Basic/BuiltinsX86.def and include/clang/Basic/BuiltinsX86_64.def

craig.topper edited edge metadata.Sep 11 2017, 9:20 AM

I think when you uploaded the changes to remove it from BuiltinsX86.def you lost your earlier changes to the header files

craig.topper added inline comments.Sep 12 2017, 12:29 AM
include/clang/Basic/BuiltinsX86.def
981 ↗(On Diff #114765)

I think you patch removed the only use of __builtin_ia32_pbroadcastq512_mem_mask right? Does your change work properly in 32-bit mode?

lib/Headers/avx512bwintrin.h
2031 ↗(On Diff #114765)

We usually don't declare variables in the intrinsics if we can avoid it. Just nest the calls.

test/CodeGen/avx512vl-builtins.c
4511 ↗(On Diff #114765)

The first line is over indented

4525 ↗(On Diff #114765)

The first line is overindented

some very minor whitespace/indentation issues

please can you confirm @craig.topper's query about __builtin_ia32_pbroadcastq512_mem_mask

lib/Headers/avx512vlintrin.h
5727 ↗(On Diff #114836)

weird indentation?

5734 ↗(On Diff #114836)

weird indentation?

5741 ↗(On Diff #114836)

weird indentation?

5748 ↗(On Diff #114836)

weird indentation?

test/CodeGen/avx512f-builtins.c
7732 ↗(On Diff #114836)

weird indentation?

7755 ↗(On Diff #114836)

weird indentation?

7966 ↗(On Diff #114836)

weird indentation?

test/CodeGen/avx512vl-builtins.c
4597 ↗(On Diff #114836)

weird indentation?

jina.nahias added inline comments.Sep 13 2017, 12:13 AM
include/clang/Basic/BuiltinsX86.def
981 ↗(On Diff #114765)

yes for both of the questions, i have deleted __builtin_ia32_pbroadcastq512_mem_mask you can see it in the new update.

I'm going to go ahead and remove __builtin_ia32_pbroadcastq512_mem_mask from clang and change _mm512_maskz_set1_epi64 to be disabled in 32-bit mode. I want to nominate this for 5.0.1 because using it in 32-bit mode causes the compile to throw a cannot select error. So disabling it in the header at least gives a better user experience.

After that goes in you should rebase this patch and enable all of the set1_epi64 intrinsics to work in 32-bit mode like they should.

lib/Headers/avx512fintrin.h
9742 ↗(On Diff #114978)

Please remove the #ifdef x86_64 from this. It should work in 32-bits as well.

lib/Headers/avx512vlintrin.h
5759 ↗(On Diff #114978)

Please remove the x86_64 from these. They should work in 32-bit mode.

jina.nahias added inline comments.Sep 18 2017, 4:36 AM
lib/Headers/avx512fintrin.h
9742 ↗(On Diff #114978)

the current generated code for 32-bit is not optimal, 32-bit needs some more work which will be in a following patch .

This revision is now accepted and ready to land.Sep 18 2017, 9:30 AM
This revision was automatically updated to reflect the committed changes.