This is an archive of the discontinued LLVM Phabricator instance.

[X86] Support -march=graniterapids-d and update -march=graniterapids
ClosedPublic

Authored by FreddyYe on Jul 20 2023, 1:12 AM.

Diff Detail

Event Timeline

FreddyYe created this revision.Jul 20 2023, 1:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2023, 1:12 AM
FreddyYe requested review of this revision.Jul 20 2023, 1:12 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJul 20 2023, 1:12 AM
Herald added subscribers: llvm-commits, Restricted Project, cfe-commits. · View Herald Transcript
FreddyYe updated this revision to Diff 542360.Jul 20 2023, 1:15 AM

add clang releasenote

RKSimon added inline comments.Jul 20 2023, 3:20 AM
clang/test/Preprocessor/predefined-arch-macros.c
1922

Maybe create a common CHECK_GNR_BASE prefix that you can check graniterapids/graniterapids-d against - currently graniterapids-d is barely testing anything

FreddyYe added inline comments.Jul 20 2023, 5:06 AM
clang/test/Preprocessor/predefined-arch-macros.c
1922
FreddyYe updated this revision to Diff 542446.Jul 20 2023, 5:27 AM
FreddyYe marked an inline comment as done.

address comment.

RKSimon added inline comments.Jul 20 2023, 6:14 AM
clang/test/Preprocessor/predefined-arch-macros.c
1925–1926

Won't this fail on the graniterapids-d run?

FreddyYe marked an inline comment as done.Jul 20 2023, 5:05 PM
FreddyYe added inline comments.
clang/test/Preprocessor/predefined-arch-macros.c
1925–1926

Whops, I didn't realize this problem before! But it indeed doesn't fail. Need to figure out why...

pengfei added inline comments.Jul 20 2023, 7:22 PM
clang/test/Preprocessor/predefined-arch-macros.c
1925–1926

I'm guessing when using multi prefixes, it will try to match with the second one if the first failed. It's common and easy to understand for positive check but a bit confusing for negative one.

FreddyYe marked 2 inline comments as done.Jul 20 2023, 10:25 PM
FreddyYe added inline comments.
clang/test/Preprocessor/predefined-arch-macros.c
1925–1926

I did some experiments and arrived at same guessing. It indeed worked here for graniterapids-d and checked each #define for it.

RKSimon added inline comments.Jul 22 2023, 6:21 AM
llvm/lib/Target/X86/X86.td
1081

Doesn't the FeatureAMXCOMPLEX need to be on GNRDAdditionalFeatures and removed from GNRAdditionalFeatures?

1081–1082

If this was incorrect it needs to be removed in its own patch

1089

Why not concat with GNRAdditionalFeatures ?

FreddyYe marked 2 inline comments as done.Jul 23 2023, 6:32 PM
FreddyYe added inline comments.
FreddyYe updated this revision to Diff 543342.Jul 23 2023, 6:45 PM
FreddyYe marked 3 inline comments as done.

Rebase and address comments.

FreddyYe added inline comments.Jul 23 2023, 6:46 PM
llvm/lib/Target/X86/X86.td
1081

Sorry for mess here. Addressed.

FreddyYe updated this revision to Diff 543344.Jul 23 2023, 7:01 PM

Update cpu_specific required features.

pengfei accepted this revision.Jul 24 2023, 5:57 AM

LGTM, but please wait one or two days for other reviewers.

llvm/lib/TargetParser/X86TargetParser.cpp
430–433

Remove the space.

This revision is now accepted and ready to land.Jul 24 2023, 5:57 AM
RKSimon accepted this revision.Jul 24 2023, 8:15 AM

LGTM - cheers

Matt added a subscriber: Matt.Jul 24 2023, 11:21 AM
skan accepted this revision.Jul 24 2023, 6:11 PM

LGTM

FreddyYe updated this revision to Diff 543797.Jul 24 2023, 7:59 PM
FreddyYe marked an inline comment as done.

address comment.

FreddyYe updated this revision to Diff 543826.Jul 24 2023, 10:46 PM

rebase and fix typo

This revision was landed with ongoing or failed builds.Jul 24 2023, 10:49 PM
This revision was automatically updated to reflect the committed changes.