d8faf03807ac implemented general-regs-only for X86 by disabling all features
with vector instructions. But the CRC32 instruction in SSE4.2 ISA, which uses
only GPRs, also becomes unavailable. This patch adds a CRC32 feature for this
instruction and allows it to be used with general-regs-only.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/X86/X86.td | ||
---|---|---|
85 | It does. But it's not a big deal in this case. The scenario described in the commit message doesn't require crc32 capable to be disabled separately. |
llvm/lib/Target/X86/X86.td | ||
---|---|---|
85 | What does gcc do? |
llvm/lib/Target/X86/X86.td | ||
---|---|---|
85 | [hjl@gnu-skx-1 gcc]$ cat /tmp/x.c #include <x86intrin.h> int foo (int x, char c) { return __crc32b (x, c); } [hjl@gnu-skx-1 gcc]$ /usr/gcc-12.0.0-x32/bin/gcc -S -O2 /tmp/x.c -S -msse4.2 [hjl@gnu-skx-1 gcc]$ /usr/gcc-12.0.0-x32/bin/gcc -S -O2 /tmp/x.c -S -mcrc32 [hjl@gnu-skx-1 gcc]$ /usr/gcc-12.0.0-x32/bin/gcc -S -O2 /tmp/x.c -S -msse4.2 -mno-crc32 In file included from /usr/gcc-12.0.0-x32/lib/gcc/x86_64-pc-linux-gnu/12.0.0/include/x86gprintrin.h:27, from /usr/gcc-12.0.0-x32/lib/gcc/x86_64-pc-linux-gnu/12.0.0/include/x86intrin.h:27, from /tmp/x.c:1: /tmp/x.c: In function ??foo??: /usr/gcc-12.0.0-x32/lib/gcc/x86_64-pc-linux-gnu/12.0.0/include/ia32intrin.h:63:1: error: inlining failed in call to ??always_inline?? ??__crc32b??: target specific option mismatch 63 | __crc32b (unsigned int __C, unsigned char __V) | ^~~~~~~~ /tmp/x.c:6:10: note: called from here 6 | return __crc32b (x, c); | ^~~~~~~~~~~~~~~ [hjl@gnu-skx-1 gcc]$ |
llvm/lib/Target/X86/X86.td | ||
---|---|---|
85 | What does gcc do for an sse4.2 intrinsic that isn't crc32 with "-msse4.2 -mno-crc32"? |
llvm/lib/Target/X86/X86.td | ||
---|---|---|
85 | -mno-crc32 has no impact on non-crc32 intrinsics. |
Instead of using ImpliedFeatures, manually enable CRC32 in presence of SSE4.2.
This should mimic GCC better.
clang/lib/Basic/Targets/X86.cpp | ||
---|---|---|
159 | This doesn't seem to be true. It causes gcc to crash. https://godbolt.org/z/39rEbsejh |
clang/lib/Basic/Targets/X86.cpp | ||
---|---|---|
159 | Well I was using GCC 11.1, it compiles. The way I see it, crash means a bug (not surprising since it's trunk), and can be interpreted as incompletely defined behavior until it's fixed. Some tests on GCC trunk:
It's hard to extract some consistent underlying logic from the GCC results. |
clang/lib/Basic/Targets/X86.cpp | ||
---|---|---|
159 | I posted a patch: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575741.html |
clang/lib/Basic/Targets/X86.cpp | ||
---|---|---|
159 | @hjl.tools does that turn the crash into making -mno-crc32 into making crc32 instruction disabled? |
clang/lib/Basic/Targets/X86.cpp | ||
---|---|---|
159 | Correct. GCC issues an error now. |
- Update behavior of -msse4.2 option.
- Add test for -msse4.2 and -mno-crc32.
- Fix some format error.
clang/lib/Headers/crc32intrin.h | ||
---|---|---|
32 | Not sure about this one. We've been consistently using this brace placement in intrinsic headers. |
llvm/lib/Support/X86TargetParser.cpp | ||
---|---|---|
533 | SSE4.1 implies CRC32. But CRC32 shouldn't imply SSE4.1. |
llvm/lib/Support/X86TargetParser.cpp | ||
---|---|---|
533 | Yes. The constexpr here means FeaturesSSE4_1 implies both FeatureSSSE3 and FeaturesCRC32. |
clang/lib/Basic/Targets/X86.cpp | ||
---|---|---|
159 | It's aligned, see clang/test/Driver/x86-mcrc32.c. |
llvm/lib/Support/X86TargetParser.cpp | ||
---|---|---|
533 | CRC32 was added in SSE4.2. In LLVM this implication relationship is bidirectional, that is:
|
llvm/lib/Support/X86TargetParser.cpp | ||
---|---|---|
533 | Sorry, I mistook SSE4.1 with SSE4.2. I meant to constexpr FeatureBitset ImpliedFeaturesSSE4_2 = FeatureSSE4_1 | FeaturesCRC32; then. |
llvm/lib/Support/X86TargetParser.cpp | ||
---|---|---|
533 | This file is only used by the frontend and it creates a bidirectional relationship. It would make -msse4.2 imply -mcrc32. But it also makes -mno-crc32 imply -mno-sse4.2. Just like -mno-sse4.1 implies -mno-sse4.2. But that's not what we want. |
Then I'm OK with this change as long as Craig's comments addressed. :)
llvm/lib/Support/X86TargetParser.cpp | ||
---|---|---|
533 | Got it. Thanks for the explanation. |
clang/lib/Basic/Targets/X86.cpp | ||
---|---|---|
160 | I guess I don't understand why this is coded differently than mmx, popcnt, and xsave? |
clang/lib/Basic/Targets/X86.cpp | ||
---|---|---|
160 | Well, I just found they're functionally equivalent. |
Why doesn't this say "not explicitly disabled" like the others above?