Skip to content
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

Merged
merged 5 commits into from
Jul 20, 2024

Conversation

jyknight
Copy link
Member

@jyknight jyknight commented Jul 17, 2024

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.

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.
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:X86 labels Jul 17, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 17, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-x86

Author: James Y Knight (jyknight)

Changes

This 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:

  • (modified) clang/test/CodeGen/attr-target-x86.c (+1-1)
  • (modified) llvm/include/llvm/TargetParser/X86TargetParser.def (-2)
  • (modified) llvm/lib/TargetParser/X86TargetParser.cpp (+7-11)
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;
Copy link
Collaborator

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?

Copy link
Member Author

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).

Copy link
Member Author

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)

Copy link
Collaborator

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:

#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:

#if !defined(__X86INTRIN_H) && !defined(_MM3DNOW_H_INCLUDED)
#error "Never use <prfchwintrin.h> directly; include <x86intrin.h> or <mm3dnow.h> instead."
#endif

Copy link
Member Author

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.

@llvmbot llvmbot added the clang:headers Headers provided by Clang, e.g. for intrinsics label Jul 17, 2024
Comment on lines -178 to -179
X86_FEATURE_COMPAT(3DNOW, "3dnow", 0)
X86_FEATURE (3DNOWA, "3dnowa")
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

@FreddyLeaf FreddyLeaf Jul 18, 2024

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."

Copy link
Member Author

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...

Copy link
Member Author

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")

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - cheers

Copy link
Contributor

@phoebewang phoebewang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@jyknight jyknight changed the title Remove 3dnow from X86TargetParser. Remove 3DNow! from X86TargetParser. Jul 20, 2024
@jyknight jyknight merged commit 3c6ea7b into llvm:main Jul 20, 2024
5 of 7 checks passed
@jyknight jyknight deleted the remove-mmx-3dnow-followup branch July 20, 2024 15:28
sgundapa pushed a commit to sgundapa/upstream_effort that referenced this pull request Jul 23, 2024
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.
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics clang Clang issues not falling into any other category compiler-rt:builtins compiler-rt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants