[NFC][ARM] Split SIMD identifier away from MVE #130296

Stylie777 · 2025-03-07T16:03:32Z

Previously, the use of MVE or MVE.FP would be defined by using the ARM::AEK_SIMD identifier. SIMD relates to the Cortex-A and Cortex-R extension that enables NEON instructions, which is called MVE for Cortex-M. To enable the linking of +simd and +nosimd to +neon and -neon when using clang, MVE and MVE.FP is now be defined using a unique enum identifier, rather than using the existing identifier for SIMD.

This patch is not changing behaviour, rather the enum value used to identify the use of MVE or MVE.FP. The only tests that require updating is the unit tests related to the AEK values themselves.

Previously, the use of MVE or MVE.FP would be defined by using the `ARM::AEK_SIMD` identifier. SIMD relates to the Cortex-A and Cortex-R extension that enables NEON instructions, which is called MVE for Cortex-M. To enable the linking of `+simd` and `+nosimd` to `+neon` and `-neon` when using clang, MVE and MVE.FP can now be defined using a unique enum identifier, rather than using the existing identifier for SIMD.

llvmbot · 2025-03-07T16:04:06Z

@llvm/pr-subscribers-backend-arm

Author: Jack Styles (Stylie777)

Changes

Previously, the use of MVE or MVE.FP would be defined by using the ARM::AEK_SIMD identifier. SIMD relates to the Cortex-A and Cortex-R extension that enables NEON instructions, which is called MVE for Cortex-M. To enable the linking of +simd and +nosimd to +neon and -neon when using clang, MVE and MVE.FP can now be defined using a unique enum identifier, rather than using the existing identifier for SIMD.

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

5 Files Affected:

(modified) llvm/include/llvm/TargetParser/ARMTargetParser.def (+5-5)
(modified) llvm/include/llvm/TargetParser/ARMTargetParser.h (+2)
(modified) llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp (+1-1)
(modified) llvm/lib/Target/ARM/MCTargetDesc/ARMTargetStreamer.cpp (+1-1)
(modified) llvm/unittests/TargetParser/TargetParserTest.cpp (+3-3)

diff --git a/llvm/include/llvm/TargetParser/ARMTargetParser.def b/llvm/include/llvm/TargetParser/ARMTargetParser.def
index 6b96c3e83c8c4..ee01d30cfc68d 100644
--- a/llvm/include/llvm/TargetParser/ARMTargetParser.def
+++ b/llvm/include/llvm/TargetParser/ARMTargetParser.def
@@ -224,8 +224,8 @@ ARM_ARCH_EXT_NAME("dotprod", ARM::AEK_DOTPROD, "+dotprod", "-dotprod")
 ARM_ARCH_EXT_NAME("dsp", ARM::AEK_DSP, "+dsp", "-dsp")
 ARM_ARCH_EXT_NAME("fp", ARM::AEK_FP, {}, {})
 ARM_ARCH_EXT_NAME("fp.dp", ARM::AEK_FP_DP, {}, {})
-ARM_ARCH_EXT_NAME("mve", (ARM::AEK_DSP | ARM::AEK_SIMD), "+mve", "-mve")
-ARM_ARCH_EXT_NAME("mve.fp", (ARM::AEK_DSP | ARM::AEK_SIMD | ARM::AEK_FP),
+ARM_ARCH_EXT_NAME("mve", (ARM::AEK_DSP | ARM::AEK_MVE), "+mve", "-mve")
+ARM_ARCH_EXT_NAME("mve.fp", (ARM::AEK_DSP | ARM::AEK_MVEFP | ARM::AEK_FP),
                   "+mve.fp", "-mve.fp")
 ARM_ARCH_EXT_NAME("idiv", (ARM::AEK_HWDIVARM | ARM::AEK_HWDIVTHUMB), {}, {})
 ARM_ARCH_EXT_NAME("mp", ARM::AEK_MP, {}, {})
@@ -345,12 +345,12 @@ ARM_CPU_NAME("cortex-m33", ARMV8MMainline, FK_FPV5_SP_D16, false, ARM::AEK_DSP)
 ARM_CPU_NAME("star-mc1", ARMV8MMainline, FK_FPV5_SP_D16, false, ARM::AEK_DSP)
 ARM_CPU_NAME("cortex-m35p", ARMV8MMainline, FK_FPV5_SP_D16, false, ARM::AEK_DSP)
 ARM_CPU_NAME("cortex-m55", ARMV8_1MMainline, FK_FP_ARMV8_FULLFP16_D16, false,
-             (ARM::AEK_DSP | ARM::AEK_SIMD | ARM::AEK_FP | ARM::AEK_FP16))
+             (ARM::AEK_DSP | ARM::AEK_MVEFP | ARM::AEK_FP | ARM::AEK_FP16))
 ARM_CPU_NAME("cortex-m85", ARMV8_1MMainline, FK_FP_ARMV8_FULLFP16_D16, false,
-             (ARM::AEK_DSP | ARM::AEK_SIMD | ARM::AEK_FP | ARM::AEK_FP16 |
+             (ARM::AEK_DSP | ARM::AEK_MVEFP | ARM::AEK_FP | ARM::AEK_FP16 |
               ARM::AEK_RAS | ARM::AEK_PACBTI))
 ARM_CPU_NAME("cortex-m52", ARMV8_1MMainline, FK_FP_ARMV8_FULLFP16_D16, false,
-             (ARM::AEK_DSP | ARM::AEK_SIMD | ARM::AEK_FP | ARM::AEK_FP16 |
+             (ARM::AEK_DSP | ARM::AEK_MVEFP | ARM::AEK_FP | ARM::AEK_FP16 |
               ARM::AEK_RAS | ARM::AEK_PACBTI))
 ARM_CPU_NAME("cortex-a32", ARMV8A, FK_CRYPTO_NEON_FP_ARMV8, false, ARM::AEK_CRC)
 ARM_CPU_NAME("cortex-a35", ARMV8A, FK_CRYPTO_NEON_FP_ARMV8, false, ARM::AEK_CRC)
diff --git a/llvm/include/llvm/TargetParser/ARMTargetParser.h b/llvm/include/llvm/TargetParser/ARMTargetParser.h
index 5dbcfd3d2d693..e176714a233e0 100644
--- a/llvm/include/llvm/TargetParser/ARMTargetParser.h
+++ b/llvm/include/llvm/TargetParser/ARMTargetParser.h
@@ -61,6 +61,8 @@ enum ArchExtKind : uint64_t {
   AEK_CDECP6 = 1 << 28,
   AEK_CDECP7 = 1 << 29,
   AEK_PACBTI = 1 << 30,
+  AEK_MVE = 1ULL << 31,
+  AEK_MVEFP = 1ULL << 31,
   // Unsupported extensions.
   AEK_OS = 1ULL << 59,
   AEK_IWMMXT = 1ULL << 60,
diff --git a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
index d6a586e1f247b..1eaf569720b9b 100644
--- a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
+++ b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
@@ -12971,7 +12971,7 @@ bool ARMAsmParser::enableArchExtFeature(StringRef Name, SMLoc &ExtLoc) {
       {ARM::AEK_CRYPTO,
        {Feature_HasV8Bit},
        {ARM::FeatureCrypto, ARM::FeatureNEON, ARM::FeatureFPARMv8}},
-      {(ARM::AEK_DSP | ARM::AEK_SIMD | ARM::AEK_FP),
+      {(ARM::AEK_DSP | ARM::AEK_MVEFP | ARM::AEK_FP),
        {Feature_HasV8_1MMainlineBit},
        {ARM::HasMVEFloatOps}},
       {ARM::AEK_FP,
diff --git a/llvm/lib/Target/ARM/MCTargetDesc/ARMTargetStreamer.cpp b/llvm/lib/Target/ARM/MCTargetDesc/ARMTargetStreamer.cpp
index b0fa03a35ec04..99dd0c796ed4e 100644
--- a/llvm/lib/Target/ARM/MCTargetDesc/ARMTargetStreamer.cpp
+++ b/llvm/lib/Target/ARM/MCTargetDesc/ARMTargetStreamer.cpp
@@ -248,7 +248,7 @@ void ARMTargetStreamer::emitTargetAttributes(const MCSubtargetInfo &STI) {
         emitFPU(STI.hasFeature(ARM::FeatureFP64) ? ARM::FK_FPV5_D16
                                                  : ARM::FK_FPV5_SP_D16);
         if (STI.hasFeature(ARM::HasMVEFloatOps))
-          emitArchExtension(ARM::AEK_SIMD | ARM::AEK_DSP | ARM::AEK_FP);
+          emitArchExtension(ARM::AEK_MVEFP | ARM::AEK_DSP | ARM::AEK_FP);
       }
     } else if (STI.hasFeature(ARM::FeatureVFP4_D16_SP))
       emitFPU(STI.hasFeature(ARM::FeatureD32)
diff --git a/llvm/unittests/TargetParser/TargetParserTest.cpp b/llvm/unittests/TargetParser/TargetParserTest.cpp
index 5d771a1a153f7..25efdd189b402 100644
--- a/llvm/unittests/TargetParser/TargetParserTest.cpp
+++ b/llvm/unittests/TargetParser/TargetParserTest.cpp
@@ -507,17 +507,17 @@ INSTANTIATE_TEST_SUITE_P(
                                    "8-M.Mainline"),
         ARMCPUTestParams<uint64_t>(
             "cortex-m55", "armv8.1-m.main", "fp-armv8-fullfp16-d16",
-            ARM::AEK_HWDIVTHUMB | ARM::AEK_DSP | ARM::AEK_SIMD | ARM::AEK_FP |
+            ARM::AEK_HWDIVTHUMB | ARM::AEK_DSP | ARM::AEK_MVEFP | ARM::AEK_FP |
                 ARM::AEK_RAS | ARM::AEK_LOB | ARM::AEK_FP16,
             "8.1-M.Mainline"),
         ARMCPUTestParams<uint64_t>(
             "cortex-m85", "armv8.1-m.main", "fp-armv8-fullfp16-d16",
-            ARM::AEK_HWDIVTHUMB | ARM::AEK_DSP | ARM::AEK_SIMD | ARM::AEK_FP |
+            ARM::AEK_HWDIVTHUMB | ARM::AEK_DSP | ARM::AEK_MVEFP | ARM::AEK_FP |
                 ARM::AEK_RAS | ARM::AEK_LOB | ARM::AEK_FP16 | ARM::AEK_PACBTI,
             "8.1-M.Mainline"),
         ARMCPUTestParams<uint64_t>(
             "cortex-m52", "armv8.1-m.main", "fp-armv8-fullfp16-d16",
-            ARM::AEK_HWDIVTHUMB | ARM::AEK_DSP | ARM::AEK_SIMD | ARM::AEK_FP |
+            ARM::AEK_HWDIVTHUMB | ARM::AEK_DSP | ARM::AEK_MVEFP | ARM::AEK_FP |
                 ARM::AEK_RAS | ARM::AEK_LOB | ARM::AEK_FP16 | ARM::AEK_PACBTI,
             "8.1-M.Mainline"),
         ARMCPUTestParams<uint64_t>("iwmmxt", "iwmmxt", "none", ARM::AEK_NONE,

jthackray · 2025-03-07T16:09:35Z

So this only affects cortex-m52, cortex-m55 and cortex-m85? Is it worth noting these cores in the change description? I assume there are no other test cases that require updating?

Stylie777 · 2025-03-07T16:13:33Z

@jthackray, yes as these are the only cores that enable MVE.FP in the .def file. This paves the way for +nosimd being fully operational in a later patch, but currently MVE is reliant on the SIMD enum value, which is not ideal. So a new enum value is needed for these features, so SIMD can be handled by its own unique value. No other test cases will need changing here as there is no intended change in the behaviour.

I have updated the comment.

jthackray

Thanks, Jack. LGTM.

davemgreen

It looks like there is a failing test. This sounds OK so long as you have tested it to make sure it doesn't alter anything unexpected. SIMD is really a generic term for any vectorization, ASIMD (Advanced SIMD) is the architectural name for Neon. It looks like for gcc +simd only implies +neon though, it can't be used for +mve too.

davemgreen · 2025-03-08T14:55:10Z

llvm/include/llvm/TargetParser/ARMTargetParser.h

+  AEK_MVE = 1ULL << 31,
+  AEK_MVEFP = 1ULL << 31,


Should these both be 31?

No, and on reflection we only need one identifier here, as MVE.FP can be defined by having both MVE and FP.

Stylie777 · 2025-03-10T14:42:49Z

@davemgreen Regarding the failing test, this is actually fixed by a PR I had planned to follow this one, to make +nosimd operational. I am going to close this PR and merge these changes with the one that enables +nosimd. Its not ideal, I would rather they had been separate, but with the failing test it makes sense to do so as to not regress the behaviour of +nosimd (not that it works anyway).

Previously, the use of MVE or MVE.FP would be defined by using the `ARM::AEK_SIMD` identifier. SIMD relates to the Cortex-A and Cortex-R extension that enables NEON instructions, which is called MVE for Cortex-M. To enable the linking of `+simd` and `+nosimd` to `+neon` and `-neon` when using clang, MVE and MVE.FP can now be defined using a unique enum identifier, rather than using the existing identifier for SIMD. This was originally planned to be merged as part of llvm#130296 but the changes made `+nosimd` an invalid argument, which, while not having any functionality, was allowed in previous versions of LLVM. To avoid regressions being introduced, this has been combined with the fix for `+nosimd` on AArch32.

Stylie777 requested review from simpal01, jthackray and davemgreen and removed request for simpal01 and jthackray March 7, 2025 16:03

llvmbot added the backend:ARM label Mar 7, 2025

Stylie777 requested a review from jthackray March 7, 2025 16:03

jthackray approved these changes Mar 7, 2025

View reviewed changes

davemgreen reviewed Mar 9, 2025

View reviewed changes

Stylie777 closed this Mar 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[NFC][ARM] Split SIMD identifier away from MVE #130296

[NFC][ARM] Split SIMD identifier away from MVE #130296

Uh oh!

Stylie777 commented Mar 7, 2025 •

edited

Loading

Uh oh!

llvmbot commented Mar 7, 2025

Uh oh!

jthackray commented Mar 7, 2025

Uh oh!

Stylie777 commented Mar 7, 2025 •

edited

Loading

Uh oh!

jthackray left a comment

Uh oh!

davemgreen left a comment

Uh oh!

davemgreen Mar 8, 2025

Uh oh!

Stylie777 Mar 10, 2025

Uh oh!

Stylie777 commented Mar 10, 2025

Uh oh!

Uh oh!

[NFC][ARM] Split SIMD identifier away from MVE #130296

[NFC][ARM] Split SIMD identifier away from MVE #130296

Uh oh!

Conversation

Stylie777 commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 7, 2025

Uh oh!

jthackray commented Mar 7, 2025

Uh oh!

Stylie777 commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jthackray left a comment

Choose a reason for hiding this comment

Uh oh!

davemgreen left a comment

Choose a reason for hiding this comment

Uh oh!

davemgreen Mar 8, 2025

Choose a reason for hiding this comment

Uh oh!

Stylie777 Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

Stylie777 commented Mar 10, 2025

Uh oh!

Uh oh!

Stylie777 commented Mar 7, 2025 •

edited

Loading

Stylie777 commented Mar 7, 2025 •

edited

Loading