This is an archive of the discontinued LLVM Phabricator instance.

[x86] Support 3 builtin functions for 32-bits targets
ClosedPublic

Authored by xiangzhangllvm on Apr 19 2022, 10:55 PM.

Details

Summary

To meet customers' requirement of building their libs in 32 bits targets:
Support 3 builtin functions

_mm_cvtsi128_si64
_mm_cvtsi64_si128
_mm_extract_epi64

for 32-bits targets.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 10:55 PM
xiangzhangllvm requested review of this revision.Apr 19 2022, 10:55 PM
LuoYuanke added inline comments.Apr 19 2022, 11:17 PM
clang/lib/Headers/emmintrin.h
3287

Not sure why we enable the intrinsics only in x86_64 mode before.

clang/lib/Headers/emmintrin.h
3287

I see ICC only support them in 64 bits at first, then extend to 32bits too.

craig.topper added inline comments.Apr 19 2022, 11:40 PM
clang/include/clang/Basic/BuiltinsX86_64.def
45

Why not the vec_set function?

47

What about the 256-bit version?

clang/lib/Headers/emmintrin.h
3293

64 bits -> 64-bit mode

clang/test/CodeGen/X86/sse2-builtins.c
545

Do we need the X86 prefix because of the x86-64 #ifdefs? Or are there other differences?

If it's just the x86-64, can we add -check-prefixes=CHECK,X64 to the x86-64 run lines and use X64 for the x86-64 only functions. That way CHECK can be used for all the common tests.

LuoYuanke added inline comments.Apr 19 2022, 11:46 PM
clang/lib/Headers/emmintrin.h
3287

I think it is because we use 64 bit GPR.

clang/include/clang/Basic/BuiltinsX86_64.def
45

This patch is to quickly fix customers' lib building problems.
So for the other builtins not required by them this time, I didn't change.

I think there must be quite number of other similar builtins need to support in 32 bits mode. We need a long way to check them.

clang/test/CodeGen/X86/sse2-builtins.c
545

Before I change the test, it only build with "-triple=x86_64", So all the CHECK should be X64 prefix.
So I add X86 prefix to just let "RUN ... -triple=i386" only check the updated 3 builtins. (let the change be small).

RKSimon added inline comments.Apr 20 2022, 12:47 AM
clang/test/CodeGen/X86/sse2-builtins.c
545

I'd much prefer we have complete test check prefix coverage for every RUN - and tbh we should be properly testing 32-bit on every x86 intrinsic test file.

xiangzhangllvm added inline comments.Apr 20 2022, 1:02 AM
clang/test/CodeGen/X86/sse2-builtins.c
545

Yes, testing 32-bit on every x86 intrinsic test file is make sense. I also confuse why this test not testing the 32-bit mode before. I think it is "defect" for the test.
But how can I well update the test by on checking the 3 updated intrinsics. Because it is strange to update the other intrinsics checking when I only update 3 intrinsics in clang.

xiangzhangllvm added inline comments.Apr 20 2022, 1:45 AM
clang/test/CodeGen/X86/sse2-builtins.c
545

Hi @craig.topper , @RKSimon, if the 32 and 64 has common prefix "CHECK", it means the line 4 (32 bits) need to check all other intrinsics. That means I need to updated a lot check string for the 32 bit mode.
What's more, currently we have no tools to auto generate the checking code for clang test.

LiuChen3 added inline comments.Apr 20 2022, 1:53 AM
clang/test/CodeGen/X86/sse2-builtins.c
545

Actually we have 'update_cc_test_checks.py', but little use.

xiangzhangllvm added inline comments.Apr 20 2022, 2:31 AM
clang/test/CodeGen/X86/sse2-builtins.c
545

I do "./llvm/utils/update_cc_test_checks.py clang/test/CodeGen/X86/sse41-builtins.c"
It generate nothing.

I'm updating the sse builtin test files to include i386 coverage - should be done in an hour or so

OK - SSE2/SSE41 now have i386 coverage - please can you rebase and update the checks to use CHECK/X64/X86 ?

OK - SSE2/SSE41 now have i386 coverage - please can you rebase and update the checks to use CHECK/X64/X86 ?

Hi @RKSimon, I very appreciate your help to update the test! You are very kind! In fact, I should do it before this patch. Thank you very much!

xiangzhangllvm marked an inline comment as done.
xiangzhangllvm added inline comments.
clang/test/CodeGen/X86/sse2-builtins.c
545–547

Sorry I need to update it to "CHECK", let me update.

xiangzhangllvm marked an inline comment as done.
craig.topper added inline comments.Apr 20 2022, 9:15 PM
clang/lib/Headers/emmintrin.h
3293

64 bits -> 64-bit

xiangzhangllvm marked an inline comment as done.
xiangzhangllvm added inline comments.
clang/lib/Headers/emmintrin.h
3293

Thank you!

RKSimon accepted this revision.Apr 21 2022, 3:56 AM

LGTM with one minor typo

clang/lib/Headers/emmintrin.h
3293

64-bit (missing hypen)

This revision is now accepted and ready to land.Apr 21 2022, 3:56 AM
xiangzhangllvm marked an inline comment as done.Apr 21 2022, 5:32 PM
xiangzhangllvm added inline comments.
clang/lib/Headers/emmintrin.h
3293

Let me directly add it when push.
And I see a test failed duo to clang-format. I'll commit a patch only do clang format for this 2 tests. (NFC)
Thank you !

This revision was landed with ongoing or failed builds.Apr 21 2022, 8:29 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2022, 8:29 PM

close with clang format at
commit 6454ff35e0e7b0c0762c640031aa6c2b5d1f16ec
[Clang Format] emmintrin.h smmintrin.h (NFC)