-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[X86] Enable APX and AVX10.2 on NVL #168061
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-x86 Author: Mikołaj Piróg (mikolaj-pirog) ChangesPer Intel Architecture Instruction Set Extensions Programming Reference rev. 60 (https://cdrdv2.intel.com/v1/dl/getContent/671368), table 1-2, NVL supports APX and AVX10.2 Full diff: https://github.com/llvm/llvm-project/pull/168061.diff 2 Files Affected:
diff --git a/llvm/lib/Target/X86/X86.td b/llvm/lib/Target/X86/X86.td
index 9e291a6ae431f..27ec052cfda40 100644
--- a/llvm/lib/Target/X86/X86.td
+++ b/llvm/lib/Target/X86/X86.td
@@ -1334,8 +1334,18 @@ def ProcessorFeatures {
!listremove(ARLSFeatures, [FeatureWIDEKL]);
// Novalake
+ list<SubtargetFeature> NVLAdditionalFeatures = [FeatureAVX10_2,
+ FeatureMOVRS,
+ FeatureEGPR,
+ FeaturePush2Pop2,
+ FeaturePPX,
+ FeatureNF,
+ FeatureNDD,
+ FeatureZU,
+ FeatureCCMP,
+ FeaturePREFETCHI];
list<SubtargetFeature> NVLFeatures =
- !listconcat(PTLFeatures, [FeaturePREFETCHI]);
+ !listconcat(PTLFeatures, NVLAdditionalFeatures);
// Clearwaterforest
list<SubtargetFeature> CWFAdditionalFeatures = [FeaturePREFETCHI,
diff --git a/llvm/lib/TargetParser/X86TargetParser.cpp b/llvm/lib/TargetParser/X86TargetParser.cpp
index 37e8ad986aa55..ad0d7b38ae1b3 100644
--- a/llvm/lib/TargetParser/X86TargetParser.cpp
+++ b/llvm/lib/TargetParser/X86TargetParser.cpp
@@ -176,7 +176,9 @@ constexpr FeatureBitset FeaturesArrowlakeS =
constexpr FeatureBitset FeaturesPantherlake =
(FeaturesArrowlakeS ^ FeatureWIDEKL);
constexpr FeatureBitset FeaturesNovalake =
- FeaturesPantherlake | FeaturePREFETCHI;
+ FeaturesPantherlake | FeaturePREFETCHI | FeatureAVX10_2 | FeatureMOVRS |
+ FeatureEGPR | FeatureZU | FeatureCCMP | FeaturePush2Pop2 | FeaturePPX |
+ FeatureNDD | FeatureNF;
constexpr FeatureBitset FeaturesClearwaterforest =
(FeaturesSierraforest ^ FeatureWIDEKL) | FeatureAVXVNNIINT16 |
FeatureSHA512 | FeatureSM3 | FeatureSM4 | FeaturePREFETCHI | FeatureUSERMSR;
|
|
Missing tests? |
My bad, I've fixed the test |
| // CHECK_ARL_M32: #define __ADX__ 1 | ||
| // CHECK_ARL_M32: #define __AES__ 1 | ||
| // CHECK_ARL_M32: #define __AVX2__ 1 | ||
| // CHECK_ARL_M32-NOT: AVX512 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this below AVX2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If avx2 is on the 2536 line the test fails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, it's a bit tricky to leverage order to solve NVL inheritance problem. Do we have a better choice? Otherwise, add a comment for future reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think twice, I found we will lose coverage for all targets, because AVX512 won't be in front of AVX2, no matter they support AVX512 or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no other way around, it's better just to remove it.
| // CHECK_ARL_M32-NOT: AVX512 | ||
| // CHECK_NVL_M32: #define __AVX10_1__ 1 | ||
| // CHECK_NVL_M32: #define __AVX10_2__ 1 | ||
| // AVX2 needs to be here, not after AES because otherwise NVL fails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the comment for // CHECK_ARL_M32-NOT: AVX512?
phoebewang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/138/builds/21922 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/27595 Here is the relevant piece of the build log for the reference |
Per Intel Architecture Instruction Set Extensions Programming Reference rev. 60 (https://cdrdv2.intel.com/v1/dl/getContent/671368), table 1-2, NVL supports APX and AVX10.2