Page MenuHomePhabricator

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

Authored by tyomitch on Sep 17 2015, 8:21 AM.

Details

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.

Diff Detail

Event Timeline

tyomitch updated this revision to Diff 34999.Sep 17 2015, 8:21 AM
tyomitch retitled this revision from to [ARM] Handle +t2dsp feature as an ArchExtKind in ARMTargetParser.def.
tyomitch updated this object.
tyomitch added reviewers: labrinea, rengolin.
tyomitch added a subscriber: llvm-commits.
psmith added a subscriber: psmith.Sep 17 2015, 9:29 AM

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 edited edge metadata.Sep 18 2015, 4:38 AM

Is DSP support mandatory for v5+?

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

This revision was automatically updated to reflect the committed changes.
tyomitch edited edge metadata.Sep 21 2015, 1:53 PM
tyomitch added a subscriber: jmolloy.

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?

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.

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.

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 updated this revision to Diff 35480.Sep 23 2015, 4:47 AM
tyomitch removed rL LLVM as the repository for this revision.
tyomitch marked an inline comment as done.Sep 23 2015, 4:49 AM

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.

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 edited edge metadata.Sep 24 2015, 1:39 AM

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

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

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 added inline comments.
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 added inline comments.Sep 25 2015, 5:57 AM
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.

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 added inline comments.Sep 29 2015, 2:35 AM
include/llvm/Support/ARMTargetParser.def
76

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