Page MenuHomePhabricator

[X86] Add CRC32 feature.
ClosedPublic

Authored by tianqing on Jul 6 2021, 12:02 AM.

Details

Summary

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.

Diff Detail

Event Timeline

tianqing created this revision.Jul 6 2021, 12:02 AM
tianqing requested review of this revision.Jul 6 2021, 12:02 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 6 2021, 12:02 AM
craig.topper added inline comments.Jul 6 2021, 12:26 AM
llvm/lib/Support/X86TargetParser.cpp
534

Same question.

llvm/lib/Target/X86/X86.td
85

Doesn't this make -mno-crc32 disable sse4.2? Is that what we want?

Or should we be doing this like popcnt where we loosely enable it at the end of X86TargetInfo::initFeatureMap

tianqing added inline comments.Jul 6 2021, 12:40 AM
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.

craig.topper added inline comments.Jul 6 2021, 7:49 AM
llvm/lib/Target/X86/X86.td
85

What does gcc do?

hjl.tools added inline comments.Jul 6 2021, 8:15 AM
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]$
craig.topper added inline comments.Jul 6 2021, 8:16 AM
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"?

hjl.tools added inline comments.Jul 6 2021, 8:19 AM
llvm/lib/Target/X86/X86.td
85

-mno-crc32 has no impact on non-crc32 intrinsics.

tianqing updated this revision to Diff 360338.Jul 20 2021, 6:39 PM

Instead of using ImpliedFeatures, manually enable CRC32 in presence of SSE4.2.

This should mimic GCC better.

craig.topper added inline comments.Jul 20 2021, 10:05 PM
clang/lib/Basic/Targets/X86.cpp
159

This doesn't seem to be true. It causes gcc to crash. https://godbolt.org/z/39rEbsejh

tianqing added inline comments.Jul 21 2021, 12:31 AM
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:

  1. -msse4.2: Pass - sse4.2 enables crc32.
  2. -mcrc32 -mno-sse4.2: Pass - no-sse4.2 doesn't disable crc32.
  3. -msse4.2 -mno-sse4.2: Error - no-sse4.2 disables crc32.
  4. -mno-crc32 -msse4.2: Crash - undefined behavior
  5. -msse4.2 -mno-crc32: Crash - undefined behavior

It's hard to extract some consistent underlying logic from the GCC results.

hjl.tools added inline comments.Jul 21 2021, 5:24 AM
clang/lib/Basic/Targets/X86.cpp
159
craig.topper added inline comments.Jul 21 2021, 9:50 AM
clang/lib/Basic/Targets/X86.cpp
159

@hjl.tools does that turn the crash into making -mno-crc32 into making crc32 instruction disabled?

hjl.tools added inline comments.Jul 21 2021, 10:11 AM
clang/lib/Basic/Targets/X86.cpp
159

Correct. GCC issues an error now.

pengfei added inline comments.Aug 9 2021, 7:16 AM
clang/lib/Headers/crc32intrin.h
14

Better to follow Lint's suggestions.

32

ditto.

clang/lib/Headers/immintrin.h
518 ↗(On Diff #360338)

Should it be better to move together with "include <smmintrin.h>"?

clang/lib/Headers/smmintrin.h
2345

Should it be added to gprintrin.h too?

tianqing updated this revision to Diff 368589.Aug 25 2021, 2:21 AM
  • Update behavior of -msse4.2 option.
  • Add test for -msse4.2 and -mno-crc32.
  • Fix some format error.
tianqing marked 2 inline comments as done.Aug 25 2021, 2:26 AM
tianqing added inline comments.
clang/lib/Headers/crc32intrin.h
32

Not sure about this one. We've been consistently using this brace placement in intrinsic headers.

pengfei added inline comments.Aug 25 2021, 7:38 AM
clang/lib/Basic/Targets/X86.cpp
159

So we don't align with GCC regarding "1. -msse4.2: Pass - sse4.2 enables crc32."?

llvm/lib/Support/X86TargetParser.cpp
533

Can we let ImpliedFeaturesSSE4_1 = FeatureSSSE3 | FeaturesCRC32 so that we don't need to add crc32 on sse4.1 and above?

hjl.tools added inline comments.Aug 25 2021, 7:46 AM
llvm/lib/Support/X86TargetParser.cpp
533

SSE4.1 implies CRC32. But CRC32 shouldn't imply SSE4.1.

pengfei added inline comments.Aug 25 2021, 7:55 AM
llvm/lib/Support/X86TargetParser.cpp
533

Yes. The constexpr here means FeaturesSSE4_1 implies both FeatureSSSE3 and FeaturesCRC32.

tianqing added inline comments.Aug 25 2021, 8:28 AM
clang/lib/Basic/Targets/X86.cpp
159

It's aligned, see clang/test/Driver/x86-mcrc32.c.

tianqing added inline comments.Aug 25 2021, 8:32 AM
llvm/lib/Support/X86TargetParser.cpp
533

CRC32 was added in SSE4.2.

In LLVM this implication relationship is bidirectional, that is:

  • -msse4.2 implies -mcrc32
  • -mcrc32 doesn't implies -msse4.2.
  • -mno-sse4.2 doesn't implies -mno-crc32.
  • But -mno-crc32 also implies -mno-sse4.2.
craig.topper added inline comments.Aug 25 2021, 8:49 AM
clang/lib/Basic/Targets/X86.cpp
158

Why doesn't this say "not explicitly disabled" like the others above?

clang/lib/Headers/ia32intrin.h
19

Is __DEFAULT_FN_ATTRS_SSE42 dead now?

clang/lib/Headers/smmintrin.h
2356

Was min vector width incorrectly being applied to these before?

pengfei added inline comments.Aug 25 2021, 6:21 PM
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.
I see you make "-msse4.2 implies -mcrc32" by FE. Changing here should make it implies in backend, so that you don't need to explicitly add crc32 in LLVM tests.

craig.topper added inline comments.Aug 25 2021, 7:02 PM
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.

pengfei accepted this revision.EditedAug 25 2021, 7:07 PM

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.

This revision is now accepted and ready to land.Aug 25 2021, 7:07 PM
tianqing updated this revision to Diff 368793.Aug 25 2021, 7:26 PM

Address review comments.

tianqing marked an inline comment as done.Aug 25 2021, 7:31 PM
tianqing added inline comments.
clang/lib/Basic/Targets/X86.cpp
158

Actually what I mean was "not explicitly enabled or disabled".

clang/lib/Headers/ia32intrin.h
19

Yes.

clang/lib/Headers/immintrin.h
518 ↗(On Diff #360338)

I removed this block because it's already in x86gprintrin.h

clang/lib/Headers/smmintrin.h
2356

I think so.

craig.topper added inline comments.Aug 25 2021, 7:43 PM
clang/lib/Basic/Targets/X86.cpp
160

I guess I don't understand why this is coded differently than mmx, popcnt, and xsave?

tianqing updated this revision to Diff 368795.Aug 25 2021, 8:02 PM
tianqing marked an inline comment as done.

Use existing code in X86.cpp

tianqing marked an inline comment as done.Aug 25 2021, 8:04 PM
tianqing added inline comments.
clang/lib/Basic/Targets/X86.cpp
160

Well, I just found they're functionally equivalent.

This revision was landed with ongoing or failed builds.Mon, Sep 6, 2:25 AM
This revision was automatically updated to reflect the committed changes.
tianqing marked an inline comment as done.