[ARM] Handle +t2dsp feature as an ArchExtKind in ARMTargetParser.def

[tyomitch]

Summary

Currently, the availability of DSP instructions (ACLE 6.4.7) is handled in a hand-rolled tricky condition block in tools/clang/lib/Basic/Targets.cpp, with a FIXME: attached.

This patch changes the handling of +t2dsp to be in line with other architecture extensions.

[psmith]

Hello,

I'm concerned we might be using T2DSP inappropriately on A class targets that do not support Thumb 2.

Context:
For the A profile the DSP extensions were introduced in ARM state only in the ARMv5TE architecture. They became available in Thumb2 with the ARMv6T2 architecture.
For the M profile the DSP extensions were introduced in the v7E-M architecture.

My understanding is that T2DSP is defined as:
/// Some M architectures don't have the DSP extension (v7E-M vs. v7M)
def FeatureDSPThumb2 : SubtargetFeature<"t2dsp", "Thumb2DSP", "true",

"Supports v7 DSP instructions in Thumb2">;

I think that using AEK_T2DSP to descibe DSP extensions for Architectures that do not support Thumb2 is not ideal. In effect the definition above has become (approximately):
/// Some architectures don't have the DSP extension
def FeatureDSPThumb2 : SubtargetFeature<"t2dsp", "Thumb2DSP", "true",

"Supports DSP instructions in ARM and/or Thumb2">;

Given the above feature exists and on the assumption "t2dsp" can't be changed to a general FeatureDSP, I've not got any universally better suggestions right now.

A possibility would be to introduce:
/// Some A architectures don't have the DSP extension in ARM state (v5T vs. v5TE)
def FeatureDSPARM : SubtargetFeature<"armdsp", "ARMDSP", "true",

"Supports DSP instructions in ARM">;

This is more accurate but would require adding "armdsp" and "t2dsp" to many A class processors.

Regards

Peter

[rengolin]

Is DSP support mandatory for v5+?

[psmith]

In D12937#248554, @rengolin wrote:

Is DSP support mandatory for v5+?

DSP extensions have been mandatory on A class from ARMv5TE onwards, there are some ARMv5T processors that don't have them. For example the ARM9TDMI. For ARM v5 processors the E will be in the name if it is ARMv5TE, for example the ARM926EJ-S

[rengolin]

ok

[tyomitch]

Responding to Peter's comment above: the option 2 (adding FeatureDSPARM which can be enabled or disabled indepentently from FeatureDSPThumb2) seems a bit of overengineering for no gain: in the existing ARM architectures, the DSP extension is either available from all supported instruction sets, or from none of them.

It would be ideal to call this "FeatureDSP", and to avoid associating this extension with either of the instruction sets; but unfortunately the "+t2dsp" syntax has already been exposed as part of LLVM/Clang user interface, and there may be external scripts specifying this feature name.

Therefore, in my commit 248152 (later reverted by James) I went for option 1, that is, to update the description for FeatureDSPThumb2 to match its new purpose.

Renato et al.: is it OK to recommit this patch, including its clang-side counterpart http://reviews.llvm.org/D12938 which James also reverted?

[psmith]

I think it is probably the least worst solution right now. I'm happy to proceed.

Another possible alternative is to add a FeatureDSP, that FeatureDSPThumb2 maps on to. However I don't think that using up another SubTargetFeature is necessarily a good trade off either.

[jmolloy]

Hi Artyom,

Some comments below.

James

llvm/trunk/lib/Support/TargetParser.cpp

193 ↗

(On Diff #35236)

Why aren't you adding -t2dsp otherwise, like the other features in this file?

llvm/trunk/lib/Target/ARM/ARM.td

122 ↗

(On Diff #35236)

It doesn't seem obvious to me why this is called "t2dsp" but also affects ARM mode. Therefore a nice comment wouldn't go amiss, because not everyone will have read your email thread.

[rengolin]

In D12937#250486, @psmith wrote:

I think it is probably the least worst solution right now. I'm happy to proceed.

I'd like to at least know how widespread is this option and who actually uses it. I'd seriously like to consider deprecating it and calling just "dsp".

Another possible alternative is to add a FeatureDSP, that FeatureDSPThumb2 maps on to. However I don't think that using up another SubTargetFeature is necessarily a good trade off either.

We could easily just change the internal table-gen feature name to FeatureDSP, and make the enum tag name just DSP and keep only the clang string t2dsp, and put a warning on the next release notes that we'll deprecate it on the next++.

[tyomitch]

In D12937#251282, @rengolin wrote:

We could easily just change the internal table-gen feature name to FeatureDSP, and make the enum tag name just DSP and keep only the clang string t2dsp, and put a warning on the next release notes that we'll deprecate it on the next++.

Done; updated the patch.

[rengolin]

Hi Alexandros,

The patch looks a lot better, thanks! Did you do a search on who's using that flag? If this is Darwin, we should ask Darwin folks before we deprecate or change their flags. If not, it'd be good to understand who and why, so we can plan accordingly.

Cheers,
Renato

[labrinea]

@rengolin added a comment
Hi Alexandros,
The patch looks a lot better, thanks! Did you do a search on who's using that flag? If this is Darwin, we should ask Darwin folks before we deprecate or change their flags. If not, it'd be good to understand who and why, so we can plan accordingly.

Hi Renato,
I guess you are addressing to @tyomitch

Artyom,
Could you investigate where is the clang string t2dsp currently being used, so that people are aware of its deprecation with this patch?

[rengolin]

Ops. Sorry, too many patches to review. :-)

[rengolin]

Despite my last comment asking for some research, I'm happy with the patch the way it is now. Renaming the flag can happen later. Thanks for the work on this, LGTM.

[richard.barton.arm]

include/llvm/Support/ARMTargetParser.def

76

Are these two lines right? The Z part of ARMv6Z means security, so I think the value should be (AEK_SEC | AEK_DSP) no?

[tyomitch]

include/llvm/Support/ARMTargetParser.def

76

You're certainly right; but these two AEK_SEC were, in fact, no-ops: in the only place where LLVM checks for AEK_SEC, it also checks for Feature_HasV7, accompanied with a "// FIXME: Also available in ARMv6-K"

I'm tempted to leave this as is now, given that reinstating the AEK_SEC would be a NFC with no obvious gain.

[richard.barton.arm]

include/llvm/Support/ARMTargetParser.def

76

That FIXME is wrong isn't it? Should be ARMv6Z. Seems like we would want to remove a FIXME that we can now remove.

[tyomitch]

include/llvm/Support/ARMTargetParser.def

76

Continued at http://reviews.llvm.org/D13236

[rengolin]

Sorry, yes, this patch looks good. Please, commit. The renaming shall be discussed on the other review, and can take a lot longer. Thanks!

include/llvm/Support/ARMTargetParser.def

ARM_ARCH("armv3m", AK_ARMV3M, "3M", "v3m", ARMBuildAttrs::CPUArch::Pre_v4,
           AEK_NONE)
 ARM_ARCH("armv4", AK_ARMV4, "4", "v4", ARMBuildAttrs::CPUArch::v4,
           AEK_NONE)
 ARM_ARCH("armv4t", AK_ARMV4T, "4T", "v4t", ARMBuildAttrs::CPUArch::v4T,
           AEK_NONE)
 ARM_ARCH("armv5t", AK_ARMV5T, "5T", "v5", ARMBuildAttrs::CPUArch::v5T,
           AEK_NONE)
 ARM_ARCH("armv5te", AK_ARMV5TE, "5TE", "v5e", ARMBuildAttrs::CPUArch::v5TE,
-          AEK_NONE)
+          AEK_T2DSP)
 ARM_ARCH("armv5tej", AK_ARMV5TEJ, "5TEJ", "v5e", ARMBuildAttrs::CPUArch::v5TEJ,
-          AEK_NONE)
+          AEK_T2DSP)
 ARM_ARCH("armv6", AK_ARMV6, "6", "v6", ARMBuildAttrs::CPUArch::v6,
-          AEK_NONE)
+          AEK_T2DSP)
 ARM_ARCH("armv6k", AK_ARMV6K, "6K", "v6k", ARMBuildAttrs::CPUArch::v6K,
-          AEK_NONE)
+          AEK_T2DSP)
 ARM_ARCH("armv6t2", AK_ARMV6T2, "6T2", "v6t2", ARMBuildAttrs::CPUArch::v6T2,
-          AEK_NONE)
+          AEK_T2DSP)
 ARM_ARCH("armv6z", AK_ARMV6Z, "6Z", "v6z", ARMBuildAttrs::CPUArch::v6KZ,
-          AEK_SEC)
+          AEK_T2DSP)
 ARM_ARCH("armv6zk", AK_ARMV6ZK, "6ZK", "v6zk", ARMBuildAttrs::CPUArch::v6KZ,
-          AEK_SEC)
+          AEK_T2DSP)
 ARM_ARCH("armv6-m", AK_ARMV6M, "6-M", "v6m", ARMBuildAttrs::CPUArch::v6_M,
           AEK_NONE)
 ARM_ARCH("armv6s-m", AK_ARMV6SM, "6S-M", "v6sm", ARMBuildAttrs::CPUArch::v6S_M,
           AEK_NONE)
 ARM_ARCH("armv7-a", AK_ARMV7A, "7-A", "v7", ARMBuildAttrs::CPUArch::v7,
-          AEK_NONE)
+          AEK_T2DSP)
 ARM_ARCH("armv7-r", AK_ARMV7R, "7-R", "v7r", ARMBuildAttrs::CPUArch::v7,
-          AEK_HWDIV)
+          (AEK_HWDIV | AEK_T2DSP))
 ARM_ARCH("armv7-m", AK_ARMV7M, "7-M", "v7m", ARMBuildAttrs::CPUArch::v7,
           AEK_HWDIV)
 ARM_ARCH("armv7e-m", AK_ARMV7EM, "7E-M", "v7em", ARMBuildAttrs::CPUArch::v7E_M,
-          AEK_HWDIV)
+          (AEK_HWDIV | AEK_T2DSP))
 ARM_ARCH("armv8-a", AK_ARMV8A, "8-A", "v8", ARMBuildAttrs::CPUArch::v8,
-         (AEK_SEC | AEK_MP | AEK_VIRT | AEK_HWDIVARM | AEK_HWDIV))
+         (AEK_SEC | AEK_MP | AEK_VIRT | AEK_HWDIVARM | AEK_HWDIV | AEK_T2DSP))
 ARM_ARCH("armv8.1-a", AK_ARMV8_1A, "8.1-A", "v8.1a", ARMBuildAttrs::CPUArch::v8,
-         (AEK_SEC | AEK_MP | AEK_VIRT | AEK_HWDIVARM | AEK_HWDIV))
+         (AEK_SEC | AEK_MP | AEK_VIRT | AEK_HWDIVARM | AEK_HWDIV | AEK_T2DSP))
 // Non-standard Arch names.
 ARM_ARCH("iwmmxt", AK_IWMMXT, "iwmmxt", "", ARMBuildAttrs::CPUArch::v5TE,
           AEK_NONE)
 ARM_ARCH("iwmmxt2", AK_IWMMXT2, "iwmmxt2", "", ARMBuildAttrs::CPUArch::v5TE,
           AEK_NONE)
 ARM_ARCH("xscale", AK_XSCALE, "xscale", "", ARMBuildAttrs::CPUArch::v5TE,
           AEK_NONE)
 ARM_ARCH("armv5", AK_ARMV5, "5T", "v5", ARMBuildAttrs::CPUArch::v5T,
           AEK_NONE)
 ARM_ARCH("armv5e", AK_ARMV5E, "5TE", "v5e", ARMBuildAttrs::CPUArch::v5TE,
-          AEK_NONE)
+          AEK_T2DSP)
 ARM_ARCH("armv6j", AK_ARMV6J, "6J", "v6", ARMBuildAttrs::CPUArch::v6,
-          AEK_NONE)
+          AEK_T2DSP)
 ARM_ARCH("armv6hl", AK_ARMV6HL, "6-M", "v6hl", ARMBuildAttrs::CPUArch::v6_M,
           AEK_NONE)
 ARM_ARCH("armv7", AK_ARMV7, "7", "v7", ARMBuildAttrs::CPUArch::v7,
           AEK_NONE)
 ARM_ARCH("armv7l", AK_ARMV7L, "7-L", "v7l", ARMBuildAttrs::CPUArch::v7,
-          AEK_NONE)
+          AEK_T2DSP)
 ARM_ARCH("armv7hl", AK_ARMV7HL, "7-L", "v7hl", ARMBuildAttrs::CPUArch::v7,
-          AEK_NONE)
+          AEK_T2DSP)
 ARM_ARCH("armv7s", AK_ARMV7S, "7-S", "v7s", ARMBuildAttrs::CPUArch::v7,
-          AEK_NONE)
+          AEK_T2DSP)
 ARM_ARCH("armv7k", AK_ARMV7K, "7-K", "v7k", ARMBuildAttrs::CPUArch::v7,
-          AEK_NONE)
+          AEK_T2DSP)
 #undef ARM_ARCH
 
 #ifndef ARM_ARCH_EXT_NAME
 #define ARM_ARCH_EXT_NAME(NAME, ID)
 #endif
 ARM_ARCH_EXT_NAME("invalid", AEK_INVALID)
 ARM_ARCH_EXT_NAME("none", AEK_NONE)
 ARM_ARCH_EXT_NAME("crc", AEK_CRC)

include/llvm/Support/TargetParser.h

enum ArchExtKind : unsigned {
   AEK_CRYPTO = 0x4,
   AEK_FP = 0x8,
   AEK_HWDIV = 0x10,
   AEK_HWDIVARM = 0x20,
   AEK_MP = 0x40,
   AEK_SIMD = 0x80,
   AEK_SEC = 0x100,
   AEK_VIRT = 0x200,
+  AEK_T2DSP = 0x400,
   // Unsupported extensions.
   AEK_OS = 0x8000000,
   AEK_IWMMXT = 0x10000000,
   AEK_IWMMXT2 = 0x20000000,
   AEK_MAVERICK = 0x40000000,
   AEK_XSCALE = 0x80000000,
 };
 

lib/Support/TargetParser.cpp

bool llvm::ARM::getExtensionFeatures(unsigned Extensions,
   if (Extensions == ARM::AEK_INVALID)
     return false;
 
   if (Extensions & ARM::AEK_CRC)
     Features.push_back("+crc");
   else
     Features.push_back("-crc");
 
+  if (Extensions & ARM::AEK_T2DSP)
+    Features.push_back("+t2dsp");
+
   return getHWDivFeatures(Extensions, Features);
 }
 
 bool llvm::ARM::getFPUFeatures(unsigned FPUKind,
                                std::vector<const char *> &Features) {
 
   if (FPUKind >= ARM::FK_LAST || FPUKind == ARM::FK_INVALID)
     return false;