This is an archive of the discontinued LLVM Phabricator instance.

[NFC][clang] Move remaining part of X86Target.def to llvm/Support/X86TargetParser.def
ClosedPublic

Authored by a.elovikov on Aug 19 2021, 4:17 PM.

Diff Detail

Event Timeline

a.elovikov requested review of this revision.Aug 19 2021, 4:17 PM
a.elovikov created this revision.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 19 2021, 4:17 PM

I'm unopposed here. Craig, what do you think?

RKSimon added inline comments.
llvm/include/llvm/Support/X86TargetParser.def
220

what commented out features?

erichkeane added inline comments.Aug 20 2021, 7:52 AM
llvm/include/llvm/Support/X86TargetParser.def
220

So this is copy/pasted from: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/X86Target.def#L70 (where it will probably be removed in a future patch?).

That comment came from here: https://reviews.llvm.org/D47474

If you look at the 'diff' here: https://reviews.llvm.org/D47474?vs=148894&id=156484#toc (1st patch vs last) You can see that the original used a bitmask to create the values rather than the string list (as suggested by @craig.topper in the review). That version had some commented out in the bitmasks.

However, I never removed the comment! So this comment likely should just be deleted.

I'm not opposed either.

llvm/include/llvm/Support/X86TargetParser.def
220

I'm not sure the commented out features made it over. For example, FEATURE_TSX was one of the commented out values, but +tsx doesn't appear in these strings.

There's nothing later than CannonLake here - does Intel need to at least reference up to Tiger/Rocketlake?

There's nothing later than CannonLake here - does Intel need to at least reference up to Tiger/Rocketlake?

@LuoYuanke ^^

a.elovikov added inline comments.Aug 20 2021, 9:51 AM
llvm/include/llvm/Support/X86TargetParser.def
220

where it will probably be removed in a future patch?

Not sure what you mean - the whole file is being removed as part of this change.

erichkeane added inline comments.Aug 20 2021, 9:53 AM
llvm/include/llvm/Support/X86TargetParser.def
220

where it will probably be removed in a future patch?

Not sure what you mean - the whole file is being removed as part of this change.

Ah! Missed that! Phab seems to have changed how it displays deleted files!

Remove stale FIXME comment in the code being moved

There's nothing later than CannonLake here - does Intel need to at least reference up to Tiger/Rocketlake?

@LuoYuanke ^^

Thanks for reminding. We've supported -march=${CPU}, but forgot to update this table. We will update it.

There's nothing later than CannonLake here - does Intel need to at least reference up to Tiger/Rocketlake?

@LuoYuanke ^^

Thanks for reminding. We've supported -march=${CPU}, but forgot to update this table. We will update it.

Shall we get this patch committed first before making any changes?

Thanks for reminding. We've supported -march=${CPU}, but forgot to update this table. We will update it.

Shall we get this patch committed first before making any changes?

Yes, committing the patch first looks good to me.

This should be autogenerated from the main X86.td, otherwise this is, unfortunately only partially, duplicates that.
E.g. there isn't a single AMD CPU in there.

RKSimon accepted this revision.Aug 21 2021, 6:28 AM

LGTM - cheers

This revision is now accepted and ready to land.Aug 21 2021, 6:28 AM

This should be autogenerated from the main X86.td, otherwise this is, unfortunately only partially, duplicates that.
E.g. there isn't a single AMD CPU in there.

The intent of this list was to duplicate the list used for the cpu_dispatch feature from the Intel Compiler, which for obvious reasons didn't include AMD names. OUR implementation at least uses feature lists, directly, so the equivalent AMD cpus would work here. At the time, I contacted a few people from AMD via email and invited them to improve this list if they'd like to, but I don't believe I ever saw a patch to do so.

This revision was landed with ongoing or failed builds.Aug 24 2021, 9:17 AM
This revision was automatically updated to reflect the committed changes.