[PATCH] D105462: [X86] Add CRC32 feature.

[Pengfei Wang via Phabricator via cfe-commits]

pengfei added inline comments.


================
Comment at: clang/lib/Basic/Targets/X86.cpp:159
+  // Enable CRC32 if SSE4.2 is enabled.
+  // NOTE: In conformance with GCC behavior, CRC32 is still available even if
+  // it's explicitly disabled.
----------------
hjl.tools wrote:
> craig.topper wrote:
> > hjl.tools wrote:
> > > tianqing wrote:
> > > > craig.topper wrote:
> > > > > This doesn't seem to be true. It causes gcc to crash. https://godbolt.org/z/39rEbsejh
> > > > 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.
> > > I posted a patch: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575741.html
> > @hjl.tools does that turn the crash into making -mno-crc32 into making crc32 instruction disabled?
> Correct.  GCC issues an error now.
So we don't align with GCC regarding "1. -msse4.2: Pass - sse4.2 enables crc32."?


================
Comment at: llvm/lib/Support/X86TargetParser.cpp:531
 constexpr FeatureBitset ImpliedFeaturesSSSE3 = FeatureSSE3;
 constexpr FeatureBitset ImpliedFeaturesSSE4_1 = FeatureSSSE3;
 constexpr FeatureBitset ImpliedFeaturesSSE4_2 = FeatureSSE4_1;
----------------
Can we let `ImpliedFeaturesSSE4_1 = FeatureSSSE3 | FeaturesCRC32` so that we don't need to add `crc32` on sse4.1 and above?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105462/new/

https://reviews.llvm.org/D105462