Skip to content

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

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

Closed
wants to merge 1 commit into from

Conversation

Stylie777
Copy link
Contributor

@Stylie777 Stylie777 commented Mar 7, 2025

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.
@Stylie777 Stylie777 requested review from simpal01, jthackray and davemgreen and removed request for simpal01 and jthackray March 7, 2025 16:03
@Stylie777 Stylie777 requested a review from jthackray March 7, 2025 16:03
@llvmbot
Copy link
Member

llvmbot commented Mar 7, 2025

@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
Copy link
Contributor

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
Copy link
Contributor Author

Stylie777 commented Mar 7, 2025

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

Copy link
Contributor

@jthackray jthackray left a comment

Choose a reason for hiding this comment

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

Thanks, Jack. LGTM.

Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +64 to +65
AEK_MVE = 1ULL << 31,
AEK_MVEFP = 1ULL << 31,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these both be 31?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@Stylie777
Copy link
Contributor Author

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

@Stylie777 Stylie777 closed this Mar 10, 2025
Stylie777 added a commit to Stylie777/llvm-project that referenced this pull request Mar 10, 2025
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 added a commit to Stylie777/llvm-project that referenced this pull request Mar 12, 2025
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 added a commit to Stylie777/llvm-project that referenced this pull request Apr 14, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants