This is an archive of the discontinued LLVM Phabricator instance.

[X86] Split Subtarget ISA / Security / Tuning Feature Flags Definitions. NFC
ClosedPublic

Authored by RKSimon on Aug 3 2021, 9:38 AM.

Details

Summary

Our list of slow/fast tuning feature flags has become pretty extensive and is randomly interleaved with ISA and Security (Retpoline etc.) flags, not even based on when the ISAs/flags were introduced, making it tricky to locate them. Plus we started treating tuning flags separately some time ago, so this patch tries to group the flags to match.

I've left them mostly in the same order within each group - I'm happy to rearrange them further if there are specific ISA or Tuning flags that you think should be kept closer together.

Diff Detail

Event Timeline

RKSimon created this revision.Aug 3 2021, 9:38 AM
RKSimon requested review of this revision.Aug 3 2021, 9:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2021, 9:38 AM
pengfei added inline comments.Aug 3 2021, 8:58 PM
llvm/lib/Target/X86/X86.td
36–37

Should this be moved together with FeatureFast7ByteNOP etc.?

281–295

These seem tuning features?

307

Is it better if we can make security features and tuning features begin with e.g., MitigationXXXX and TuningXXX respectively?

craig.topper added inline comments.Aug 3 2021, 9:05 PM
llvm/lib/Target/X86/X86.td
36–37

Feature NOPL isn't a tuning feature. It's architectural.

281–295

They have CPUID bits and they appear in HSWAdditionalFeatures rather than HSWTuning. and ICLAdditionalFeatures rather than ICLTuning.

RKSimon added inline comments.Aug 4 2021, 1:34 AM
llvm/lib/Target/X86/X86.td
281–295

Yes, that was my reasoning to keep them here - I agree they (annoyingly) straddle the line between arch and tuning feature :(

307

I have no objection - a naming convention like that could help. I'd like to do that as a separate patch though.

pengfei accepted this revision.Aug 4 2021, 1:38 AM

LGTM.

This revision is now accepted and ready to land.Aug 4 2021, 1:38 AM