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

[AArch64] Make apple-m4 armv8.7-a again (from armv9.2-a). #106312

Conversation

ahmedbougacha
Copy link
Member

This is a partial revert of c66e1d6. Even though that
allowed us to declare v9.2-a support without picking up SVE2
in both the backend and the driver, the frontend itself still
enabled SVE via the arch version's default extensions.

Avoid that by reverting back to v8.7-a while we look into
longer-term solutions.

@ahmedbougacha ahmedbougacha requested a review from jroelofs August 27, 2024 23:55
@ahmedbougacha ahmedbougacha added this to the LLVM 19.X Release milestone Aug 27, 2024
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:AArch64 labels Aug 27, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 27, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-aarch64

Author: Ahmed Bougacha (ahmedbougacha)

Changes

This is a partial revert of c66e1d6. Even though that
allowed us to declare v9.2-a support without picking up SVE2
in both the backend and the driver, the frontend itself still
enabled SVE via the arch version's default extensions.

Avoid that by reverting back to v8.7-a while we look into
longer-term solutions.


Full diff: https://github.com/llvm/llvm-project/pull/106312.diff

3 Files Affected:

  • (modified) clang/test/CodeGen/aarch64-targetattr.c (+9)
  • (modified) llvm/lib/Target/AArch64/AArch64Processors.td (+4-1)
  • (modified) llvm/unittests/TargetParser/TargetParserTest.cpp (+1-1)
diff --git a/clang/test/CodeGen/aarch64-targetattr.c b/clang/test/CodeGen/aarch64-targetattr.c
index d6227be2ebef83..1bc78a6e1f8c0f 100644
--- a/clang/test/CodeGen/aarch64-targetattr.c
+++ b/clang/test/CodeGen/aarch64-targetattr.c
@@ -191,6 +191,14 @@ __attribute__((target("no-v9.3a")))
 //
 void minusarch() {}
 
+__attribute__((target("cpu=apple-m4")))
+// CHECK-LABEL: define {{[^@]+}}@applem4
+// CHECK-SAME: () #[[ATTR18:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    ret void
+//
+void applem4() {}
+
 //.
 // CHECK: attributes #[[ATTR0]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+crc,+fp-armv8,+lse,+neon,+ras,+rdm,+v8.1a,+v8.2a,+v8a" }
 // CHECK: attributes #[[ATTR1]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+crc,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rdm,+sve,+v8.1a,+v8.2a,+v8a" }
@@ -210,6 +218,7 @@ void minusarch() {}
 // CHECK: attributes #[[ATTR15]] = { noinline nounwind optnone "branch-target-enforcement" "guarded-control-stack" "no-trapping-math"="true" "sign-return-address"="non-leaf" "sign-return-address-key"="a_key" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-n1" "target-features"="+aes,+bf16,+bti,+ccidx,+complxnum,+crc,+dit,+dotprod,+flagm,+fp-armv8,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+predres,+ras,+rcpc,+rdm,+sb,+sha2,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
 // CHECK: attributes #[[ATTR16]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
 // CHECK: attributes #[[ATTR17]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="-v9.3a" }
+// CHECK: attributes #[[ATTR18]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="apple-m4" "target-features"="+aes,+bf16,+bti,+ccidx,+complxnum,+crc,+dit,+dotprod,+flagm,+fp-armv8,+fp16fml,+fpac,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+predres,+ras,+rcpc,+rdm,+sb,+sha2,+sha3,+sme,+sme-f64f64,+sme-i16i64,+sme2,+ssbs,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8.7a,+v8a,+wfxt" }
 //.
 // CHECK: [[META0:![0-9]+]] = !{i32 1, !"wchar_size", i32 4}
 // CHECK: [[META1:![0-9]+]] = !{!"{{.*}}clang version {{.*}}"}
diff --git a/llvm/lib/Target/AArch64/AArch64Processors.td b/llvm/lib/Target/AArch64/AArch64Processors.td
index 84d8cae3a0a5d1..31e3db19e3462d 100644
--- a/llvm/lib/Target/AArch64/AArch64Processors.td
+++ b/llvm/lib/Target/AArch64/AArch64Processors.td
@@ -895,7 +895,10 @@ def ProcessorFeatures {
                                      FeatureLSE, FeaturePAuth, FeatureFPAC,
                                      FeatureRAS, FeatureRCPC, FeatureRDM,
                                      FeatureBF16, FeatureDotProd, FeatureMatMulInt8, FeatureSSBS];
-  list<SubtargetFeature> AppleM4 = [HasV9_2aOps, FeatureSHA2, FeatureFPARMv8,
+  // Technically apple-m4 is ARMv9.2a, but a quirk of LLVM defines v9.0 as
+  // requiring SVE, which is optional according to the Arm ARM and not
+  // supported by the core. ARMv8.7a is the next closest choice.
+  list<SubtargetFeature> AppleM4 = [HasV8_7aOps, FeatureSHA2, FeatureFPARMv8,
                                     FeatureNEON, FeaturePerfMon, FeatureSHA3,
                                     FeatureFullFP16, FeatureFP16FML,
                                     FeatureAES, FeatureBF16,
diff --git a/llvm/unittests/TargetParser/TargetParserTest.cpp b/llvm/unittests/TargetParser/TargetParserTest.cpp
index 7d999b826252a2..13db80ab5c68ea 100644
--- a/llvm/unittests/TargetParser/TargetParserTest.cpp
+++ b/llvm/unittests/TargetParser/TargetParserTest.cpp
@@ -1122,7 +1122,7 @@ INSTANTIATE_TEST_SUITE_P(
                       AArch64CPUTestParams("apple-a16", "armv8.6-a"),
                       AArch64CPUTestParams("apple-m3", "armv8.6-a"),
                       AArch64CPUTestParams("apple-a17", "armv8.6-a"),
-                      AArch64CPUTestParams("apple-m4", "armv9.2-a"),
+                      AArch64CPUTestParams("apple-m4", "armv8.7-a"),
                       AArch64CPUTestParams("exynos-m3", "armv8-a"),
                       AArch64CPUTestParams("exynos-m4", "armv8.2-a"),
                       AArch64CPUTestParams("exynos-m5", "armv8.2-a"),

Comment on lines 898 to 900
// Technically apple-m4 is ARMv9.2a, but a quirk of LLVM defines v9.0 as
// requiring SVE, which is optional according to the Arm ARM and not
// supported by the core. ARMv8.7a is the next closest choice.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the comment to explain the "quirk". I think what you mean is that clang will still add all of the architecture default features when you are selecting a particular CPU, even if the backend wouldn't add them.

This is a partial revert of c66e1d6.  Even though that
allowed us to declare v9.2-a support without picking up SVE2
in both the backend and the driver, the frontend itself still
enabled SVE via the arch version's default extensions.

Avoid that by reverting back to v8.7-a while we look into
longer-term solutions.
@ahmedbougacha ahmedbougacha force-pushed the users/ahmedbougacha/aarch64-apple-m4-v8.7a branch from 0bec083 to 1a16e34 Compare August 29, 2024 16:49
@ahmedbougacha ahmedbougacha merged commit e5e38dd into llvm:main Aug 29, 2024
4 of 6 checks passed
@ahmedbougacha ahmedbougacha deleted the users/ahmedbougacha/aarch64-apple-m4-v8.7a branch August 29, 2024 16:50
@ahmedbougacha
Copy link
Member Author

/cherry-pick e5e38dd

@llvmbot
Copy link
Member

llvmbot commented Aug 29, 2024

Failed to cherry-pick: e5e38dd

https://github.com/llvm/llvm-project/actions/runs/10619317445

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

ahmedbougacha added a commit to ahmedbougacha/llvm-project that referenced this pull request Aug 29, 2024
This is a partial revert of c66e1d6.  Even though that
allowed us to declare v9.2-a support without picking up SVE2
in both the backend and the driver, the frontend itself still
enabled SVE via the arch version's default extensions.

Avoid that by reverting back to v8.7-a while we look into
longer-term solutions.

(cherry picked from commit e5e38dd)
@ahmedbougacha
Copy link
Member Author

#106599 release/19.x: [AArch64] Make apple-m4 armv8.7-a again (from armv9.2-a).

tru pushed a commit to ahmedbougacha/llvm-project that referenced this pull request Sep 1, 2024
This is a partial revert of c66e1d6.  Even though that
allowed us to declare v9.2-a support without picking up SVE2
in both the backend and the driver, the frontend itself still
enabled SVE via the arch version's default extensions.

Avoid that by reverting back to v8.7-a while we look into
longer-term solutions.

(cherry picked from commit e5e38dd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AArch64 clang Clang issues not falling into any other category release:cherry-pick-failed
Projects
Development

Successfully merging this pull request may close these issues.

4 participants