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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Support/X86TargetParser.cpp:531
 constexpr FeatureBitset ImpliedFeaturesSSSE3 = FeatureSSE3;
 constexpr FeatureBitset ImpliedFeaturesSSE4_1 = FeatureSSSE3;
 constexpr FeatureBitset ImpliedFeaturesSSE4_2 = FeatureSSE4_1;
----------------
pengfei wrote:
> tianqing wrote:
> > pengfei wrote:
> > > hjl.tools wrote:
> > > > pengfei wrote:
> > > > > Can we let `ImpliedFeaturesSSE4_1 = FeatureSSSE3 | FeaturesCRC32` so that we don't need to add `crc32` on sse4.1 and above?
> > > > SSE4.1 implies CRC32.  But CRC32 shouldn't imply SSE4.1.
> > > Yes. The constexpr here means `FeaturesSSE4_1` implies both `FeatureSSSE3` and `FeaturesCRC32`.
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105462