-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Remove 3DNow! from X86TargetParser. #99352
Conversation
This should've been part of PR llvm#96246, but was missed. This addresses the spurious inclusion of (now unsupported) target features '-3dnow' and '-3dnowa', when disabling mmx.
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-x86 Author: James Y Knight (jyknight) ChangesThis addresses the spurious inclusion of (now unsupported) target features '-3dnow' and '-3dnowa', when disabling mmx. It should've been part of PR #96246, but was missed. Full diff: https://github.com/llvm/llvm-project/pull/99352.diff 3 Files Affected:
diff --git a/clang/test/CodeGen/attr-target-x86.c b/clang/test/CodeGen/attr-target-x86.c
index 3c2b511157f99..b1ae6678531b9 100644
--- a/clang/test/CodeGen/attr-target-x86.c
+++ b/clang/test/CodeGen/attr-target-x86.c
@@ -64,7 +64,7 @@ void __attribute__((target("avx10.1-512"))) avx10_1_512(void) {}
// CHECK: #4 = {{.*}}"target-cpu"="i686" "target-features"="+cmov,+cx8,+x87,-avx,-avx10.1-256,-avx10.1-512,-avx2,-avx512bf16,-avx512bitalg,-avx512bw,-avx512cd,-avx512dq,-avx512f,-avx512fp16,-avx512ifma,-avx512vbmi,-avx512vbmi2,-avx512vl,-avx512vnni,-avx512vp2intersect,-avx512vpopcntdq,-avxifma,-avxneconvert,-avxvnni,-avxvnniint16,-avxvnniint8,-f16c,-fma,-fma4,-sha512,-sm3,-sm4,-sse4.1,-sse4.2,-vaes,-vpclmulqdq,-xop" "tune-cpu"="i686"
// CHECK: #5 = {{.*}}"target-cpu"="ivybridge" "target-features"="+avx,+cmov,+crc32,+cx16,+cx8,+f16c,+fsgsbase,+fxsr,+mmx,+pclmul,+popcnt,+rdrnd,+sahf,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt,-aes,-avx10.1-256,-avx10.1-512,-vaes"
// CHECK-NOT: tune-cpu
-// CHECK: #6 = {{.*}}"target-cpu"="i686" "target-features"="+cmov,+cx8,+x87,-3dnow,-3dnowa,-mmx"
+// CHECK: #6 = {{.*}}"target-cpu"="i686" "target-features"="+cmov,+cx8,+x87,-mmx"
// CHECK: #7 = {{.*}}"target-cpu"="lakemont" "target-features"="+cx8,+mmx"
// CHECK-NOT: tune-cpu
// CHECK: #8 = {{.*}}"target-cpu"="i686" "target-features"="+cmov,+cx8,+x87" "tune-cpu"="sandybridge"
diff --git a/llvm/include/llvm/TargetParser/X86TargetParser.def b/llvm/include/llvm/TargetParser/X86TargetParser.def
index 0e4ad873e3639..b078706e5f400 100644
--- a/llvm/include/llvm/TargetParser/X86TargetParser.def
+++ b/llvm/include/llvm/TargetParser/X86TargetParser.def
@@ -175,8 +175,6 @@ X86_FEATURE_COMPAT(AVX512BF16, "avx512bf16", 34)
X86_FEATURE_COMPAT(AVX512VP2INTERSECT, "avx512vp2intersect", 35)
// Below Features has some missings comparing to gcc, it's because gcc has some
// not one-to-one mapped in llvm.
-X86_FEATURE_COMPAT(3DNOW, "3dnow", 0)
-X86_FEATURE (3DNOWA, "3dnowa")
X86_FEATURE_COMPAT(ADX, "adx", 0)
X86_FEATURE (64BIT, "64bit")
X86_FEATURE_COMPAT(CLDEMOTE, "cldemote", 0)
diff --git a/llvm/lib/TargetParser/X86TargetParser.cpp b/llvm/lib/TargetParser/X86TargetParser.cpp
index 141ecb936b708..5ace0045cb0e8 100644
--- a/llvm/lib/TargetParser/X86TargetParser.cpp
+++ b/llvm/lib/TargetParser/X86TargetParser.cpp
@@ -171,14 +171,14 @@ constexpr FeatureBitset FeaturesClearwaterforest =
// Geode Processor.
constexpr FeatureBitset FeaturesGeode =
- FeatureX87 | FeatureCMPXCHG8B | FeatureMMX | Feature3DNOW | Feature3DNOWA;
+ FeatureX87 | FeatureCMPXCHG8B | FeatureMMX;
// K6 processor.
constexpr FeatureBitset FeaturesK6 = FeatureX87 | FeatureCMPXCHG8B | FeatureMMX;
// K7 and K8 architecture processors.
constexpr FeatureBitset FeaturesAthlon =
- FeatureX87 | FeatureCMPXCHG8B | FeatureMMX | Feature3DNOW | Feature3DNOWA;
+ FeatureX87 | FeatureCMPXCHG8B | FeatureMMX;
constexpr FeatureBitset FeaturesAthlonXP =
FeaturesAthlon | FeatureFXSR | FeatureSSE;
constexpr FeatureBitset FeaturesK8 =
@@ -256,8 +256,8 @@ constexpr ProcInfo Processors[] = {
// i486-generation processors.
{ {"i486"}, CK_i486, ~0U, FeatureX87, '\0', false },
{ {"winchip-c6"}, CK_WinChipC6, ~0U, FeaturesPentiumMMX, '\0', false },
- { {"winchip2"}, CK_WinChip2, ~0U, FeaturesPentiumMMX | Feature3DNOW, '\0', false },
- { {"c3"}, CK_C3, ~0U, FeaturesPentiumMMX | Feature3DNOW, '\0', false },
+ { {"winchip2"}, CK_WinChip2, ~0U, FeaturesPentiumMMX, '\0', false },
+ { {"c3"}, CK_C3, ~0U, FeaturesPentiumMMX, '\0', false },
// i586-generation processors, P5 microarchitecture based.
{ {"i586"}, CK_i586, ~0U, FeatureX87 | FeatureCMPXCHG8B, '\0', false },
{ {"pentium"}, CK_Pentium, ~0U, FeatureX87 | FeatureCMPXCHG8B, 'B', false },
@@ -386,8 +386,8 @@ constexpr ProcInfo Processors[] = {
{ {"lakemont"}, CK_Lakemont, ~0U, FeatureCMPXCHG8B, '\0', false },
// K6 architecture processors.
{ {"k6"}, CK_K6, ~0U, FeaturesK6, '\0', false },
- { {"k6-2"}, CK_K6_2, ~0U, FeaturesK6 | Feature3DNOW, '\0', false },
- { {"k6-3"}, CK_K6_3, ~0U, FeaturesK6 | Feature3DNOW, '\0', false },
+ { {"k6-2"}, CK_K6_2, ~0U, FeaturesK6, '\0', false },
+ { {"k6-3"}, CK_K6_3, ~0U, FeaturesK6, '\0', false },
// K7 architecture processors.
{ {"athlon"}, CK_Athlon, ~0U, FeaturesAthlon, '\0', false },
{ {"athlon-tbird"}, CK_Athlon, ~0U, FeaturesAthlon, '\0', false },
@@ -493,6 +493,7 @@ constexpr FeatureBitset ImpliedFeaturesFXSR = {};
constexpr FeatureBitset ImpliedFeaturesINVPCID = {};
constexpr FeatureBitset ImpliedFeaturesLWP = {};
constexpr FeatureBitset ImpliedFeaturesLZCNT = {};
+constexpr FeatureBitset ImpliedFeaturesMMX = {};
constexpr FeatureBitset ImpliedFeaturesMWAITX = {};
constexpr FeatureBitset ImpliedFeaturesMOVBE = {};
constexpr FeatureBitset ImpliedFeaturesMOVDIR64B = {};
@@ -534,11 +535,6 @@ constexpr FeatureBitset ImpliedFeaturesXSAVEC = FeatureXSAVE;
constexpr FeatureBitset ImpliedFeaturesXSAVEOPT = FeatureXSAVE;
constexpr FeatureBitset ImpliedFeaturesXSAVES = FeatureXSAVE;
-// MMX->3DNOW->3DNOWA chain.
-constexpr FeatureBitset ImpliedFeaturesMMX = {};
-constexpr FeatureBitset ImpliedFeatures3DNOW = FeatureMMX;
-constexpr FeatureBitset ImpliedFeatures3DNOWA = Feature3DNOW;
-
// SSE/AVX/AVX512F chain.
constexpr FeatureBitset ImpliedFeaturesSSE = {};
constexpr FeatureBitset ImpliedFeaturesSSE2 = FeatureSSE;
|
@@ -171,14 +171,14 @@ constexpr FeatureBitset FeaturesClearwaterforest = | |||
|
|||
// Geode Processor. | |||
constexpr FeatureBitset FeaturesGeode = | |||
FeatureX87 | FeatureCMPXCHG8B | FeatureMMX | Feature3DNOW | Feature3DNOWA; |
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.
Do we need to add a FeaturePREFTECH of some kind?
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.
Possibly not?
We didn't enable it before (so, e.g. __PRFCHW__
isn't and wasn't defined), and while we DID enable the prefetchw instruction in the backend for these targets, we still do now (via implying it in X86.td).
OTOH, maybe it's weird to have that divergence, so I'd also be fine with adding it (and therefore setting the preprocessor define).
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 somewhat lean towards submitting the commit as-is, but will change this if you wish)
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 think we should add the define. If/when we eventually remove mm3dnow.h then the only way to access prfchwintrin.h is via x86intrin.h (which we already recommend) which can sometimes require the __PRFCHW__
define:
llvm-project/clang/lib/Headers/x86intrin.h
Lines 17 to 19 in f6add66
#if !defined(__SCE__) || __has_feature(modules) || defined(__PRFCHW__) | |
#include <prfchwintrin.h> | |
#endif |
Also, we still have references to mm3dnow.h in prfchwintrin.h that probably need removing:
llvm-project/clang/lib/Headers/prfchwintrin.h
Lines 10 to 12 in f6add66
#if !defined(__X86INTRIN_H) && !defined(_MM3DNOW_H_INCLUDED) | |
#error "Never use <prfchwintrin.h> directly; include <x86intrin.h> or <mm3dnow.h> instead." | |
#endif |
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.
OK, making the change.
I note that "sometimes" is essentially "if __SCE__
is defined". Really weird to have that be Sony-specific behavior.
X86_FEATURE_COMPAT(3DNOW, "3dnow", 0) | ||
X86_FEATURE (3DNOWA, "3dnowa") |
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.
@FreddyLeaf IIRC, we need to find two features to fill here, right?
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.
Yes, if gcc also deleted 3dnow, then find 1 feature to fill here. if not, then find 2.
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.
Sorry, I realized even gcc removed, they will keep one blank here to keep backward compatibility. So two features are required.
according to this comment above the file:
// We cannot re-adjust the position of X86_FEATURE_COMPAT at the whole list.
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 think you can put sub-features of APX here if you do not have better candidates. egpr,ndd,push2pop2,ppx,ccmp,cf,nf,zu
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.
A bad news is that only zu
can be "used" here. all other sub-features of APX have already be "used."
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 don't understand this thread. What exactly needs to match what else?
Also, if it's important for each feature to be assigned a specific ABI-stable number for some reason, we should assign that number directly in this macro, not indirectly via ordering...
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.
Could I just put this in place of these two lines for now?
X86_FEATURE (DUMMYFEATURE1, "__dummyfeature1")
X86_FEATURE (DUMMYFEATURE2, "__dummyfeature2")
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've gone ahead and done that.
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.
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 - cheers
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.
This addresses the spurious inclusion of (now unsupported) target features '-3dnow' and '-3dnowa' when disabling mmx (when then caused log output from `clang -mno-mmx`). It should've been part of PR llvm#96246, but was missed. Also tweaks the warning in prfchwintrin.h to not recommend the deprecated mm3dnow.h header.
Summary: This addresses the spurious inclusion of (now unsupported) target features '-3dnow' and '-3dnowa' when disabling mmx (when then caused log output from `clang -mno-mmx`). It should've been part of PR #96246, but was missed. Also tweaks the warning in prfchwintrin.h to not recommend the deprecated mm3dnow.h header. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251255
This addresses the spurious inclusion of (now unsupported) target features '-3dnow' and '-3dnowa' when disabling mmx (when then caused log output from
clang -mno-mmx
).It should've been part of PR #96246, but was missed.
Also tweaks the warning in prfchwintrin.h to not recommend the deprecated mm3dnow.h header.