This is an archive of the discontinued LLVM Phabricator instance.

[X86] Move frontend CPU feature initialization to a look up table based implementation.
ClosedPublic

Authored by craig.topper on Jun 28 2020, 3:45 PM.

Details

Summary

This replaces the switch statement implementation in the clang's
X86.cpp with a lookup table X86TargetParser.cpp.

I've used constexpr and copy of the FeatureBitset from
SubtargetFeature.h to store the features in a lookup table.
After the lookup the bitset is translated into strings for use
by the rest of the frontend code.

I had to modify the implementation of the FeatureBitset to avoid
bugs in libgcc's constexpr handling. It seems to not like the
same array entry to be used on the left side and right hand side
of an assignment or &= or |=. I've also used uint32_t instead of
uint64_t and sized based on the X86::CPU_FEATURE_MAX.

I've initialized the features for different CPUs outside of the
table so that they can be inherited from to express inheritance in
an adhoc way. This was one of the big limitations of the switch
where had resorted to labels and gotos.

Diff Detail

Event Timeline

craig.topper created this revision.Jun 28 2020, 3:45 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 28 2020, 3:45 PM
Herald added subscribers: jfb, hiraditya. · View Herald Transcript
erichkeane accepted this revision.Jun 29 2020, 6:32 AM

A bunch of clang-format, plus 1 odd looking equation, otherwise this like good.

llvm/lib/Support/X86TargetParser.cpp
27

Why is this CPU_FEATURE_MAX + 0? Are you missing parens?

This revision is now accepted and ready to land.Jun 29 2020, 6:32 AM

-Run clang-format. Some lines in the table are still slightly over 80 columns.
-Fix the number of words in the FeatureBitset.

erichkeane accepted this revision.Jun 29 2020, 10:24 AM

@RKSimon do you mind taking a look at this too?

skan added a subscriber: skan.Jun 29 2020, 6:45 PM

LGTM with a couple of minors

clang/test/Preprocessor/predefined-arch-macros.c
2835 ↗(On Diff #274161)

The additional znver -NOT checks should definitely be split out.

llvm/lib/Support/X86TargetParser.cpp
179

Maybe don't make this dependent on PentiumMMX?

FeatureX87 | FeatureCMPXCHG8B | FeatureMMX | Feature3DNOW | Feature3DNOWA

316–318

Maybe create a FeaturesK6 and build k6-2/3 off that?