-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[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
Conversation
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.
@llvm/pr-subscribers-backend-arm Author: Jack Styles (Stylie777) ChangesPreviously, the use of MVE or MVE.FP would be defined by using the Full diff: https://github.com/llvm/llvm-project/pull/130296.diff 5 Files Affected:
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,
|
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? |
@jthackray, yes as these are the only cores that enable MVE.FP in the I have updated the comment. |
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.
Thanks, Jack. LGTM.
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.
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.
AEK_MVE = 1ULL << 31, | ||
AEK_MVEFP = 1ULL << 31, |
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.
Should these both be 31?
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.
No, and on reflection we only need one identifier here, as MVE.FP can be defined by having both MVE and FP.
@davemgreen Regarding the failing test, this is actually fixed by a PR I had planned to follow this one, to make |
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.
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.
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.
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.