- Added arch extensions in TargetParser
- Added API for retrieving the corresponding target features
- To be tested by clang
Details
- Reviewers
rengolin richard.barton.arm
Diff Detail
Event Timeline
lib/Support/TargetParser.cpp | ||
---|---|---|
368 | Changed to if (StringRef("idiv") == AE.Name) due to compiler warning |
Alexandros asked if I could look over the table of extensions, but I have some points on the code in general.
I have a few concerns about the direction we are taking here which can be summarised as:
- Do we care about GCC compatibility?
- Do we care about close adherence to ARM terminology?
For the first, I note that we are thinking of supporting the -march/mcpu=arch/cpu+{no}extn syntax for the AArch32 driver. This is great news and gcc trunk is going the same way. However, gcc are only doing this for integer extensions, and currently only "+crc" is supported here. I have not got confirmation from them whether they intend to go back and do this for pre-ARMv8 extensions, so I don't know if "+hwdivarm" or equivalent is intended to be added for example. The presence of "+fp" in this list concerns me for two reasons. Firstly, ARMv7-A and ARMv7-M have two legal FP extensions - VFPv3 or VFPv4 and VFPv4 or FPv5 respectively - so which of these does "+fp" mean when targeting one of these architectures. Secondly, from talking to the GCC guys here at ARM, they are adamant that they will not be supporting floating point extensions in the -march=arch+extn syntax mostly for legacy reasons.
With this change we are intentionally allowing more than what GCC will allow for these options. I don't think that is a problem per-se, but thought that it was worth pointing out so we are all sure we know what we are doing. I wonder whether the extension table should have a field saying which frontend (AArch64 vs ARM) each extension is permitted in as an -march=arch+extn option?
Secondly, our implementation does not adhere to the letter of the ARMARM with respect to architectural extensions. For example, "hwdiv" (as in SDIV/UDIV in T32 mode) is never an architectural extension. "hwdiv" is part of the base ARMv7-R, ARMv7-(E)M and ARMv8-A. The Division extension is an optional extension to both ARMv7-A and ARMv7-R and mandates SDIV/UDIV in both instruction sets. Therefore, "hwdiv" - i.e. T32 divide by itself, is never allowed as an architectural extension. It is either part of the base architecture or as part of the division extension alongside "hwdiv" - i.e. A32 Divide by itself. Thus a better extension would be something like "+div" which would add divide to both instruction sets were it not already present. Note also, that we would not be compatible with GCC on this option.
Again - this is not a deal breaker and the net result in terms of which instructions are available is the same. I just wanted to raise this as a concern so that we are all on the same page.
Regarding the table, apart from the above Divide concern, I notice also
- the lack of MP extension for Cortex-R5, Cortex-R7, Cortex-A5, Cortex-A7, Cortex-A9, Cortex-A12, Cortex-A15 and Cortex-A17.
- the lack of Virtualization extension for Cortex-A7, Cortex-A12, Cortex-A15 and Cortex-A17.
- the OS extension is the difference between ARMv6-M and ARMv6S-M (although I understand that ARMv6-M in clang/llvm really means ARMv6S-M, which is fine) and does not get used anywhere.
- XScale and iwmmxt being available as both architectures and extensions. Not sure if that is correct or not.
To a point. We don't want to do things differently, but it should be ok to do more, or to do less, in certain cases.
- Do we care about close adherence to ARM terminology?
When there is no legacy concerns, yes.
With this change we are intentionally allowing more than what GCC will allow for these options. I don't think that is a problem per-se, but thought that it was worth pointing out so we are all sure we know what we are doing. I wonder whether the extension table should have a field saying which frontend (AArch64 vs ARM) each extension is permitted in as an -march=arch+extn option?
Since the parser is universal, and we can support those features by adding/removing them from the target description, I see no reason to *not* have them. We'd have to *add* code to have *less* functionality. Since this is in the area where we're doing more, not different, it should be ok.
Secondly, our implementation does not adhere to the letter of the ARMARM with respect to architectural extensions. For example, "hwdiv" (as in SDIV/UDIV in T32 mode) is never an architectural extension. "hwdiv" is part of the base ARMv7-R, ARMv7-(E)M and ARMv8-A. The Division extension is an optional extension to both ARMv7-A and ARMv7-R and mandates SDIV/UDIV in both instruction sets. Therefore, "hwdiv" - i.e. T32 divide by itself, is never allowed as an architectural extension. It is either part of the base architecture or as part of the division extension alongside "hwdiv" - i.e. A32 Divide by itself. Thus a better extension would be something like "+div" which would add divide to both instruction sets were it not already present. Note also, that we would not be compatible with GCC on this option.
This is not easy to solve. We're coming from GNU-land, where the options in the assembler and command-line options already existed, and we *have* to support them. For new options, we should adhere to the ARM ARM, but for legacy ones, we need to keep compatible.
Regarding HWdiv, there is a catch. For instance, Krait has HWdiv, but it's closer to an A9 than an A15. We've added a patch that teaches that to LLVM, so if we can't have HWdiv as an extension, that won't work, and we'll produce worse code for Krait. Not to mention the legacy ".arch_extension idiv" options that we have to support.
Furthermore, HWdiv in the ARM ARM is separated between SDIV and UDIV, but what we really model is ARMDIIV and T2DIV, which is orthogonal and not clearly represented in any ARM document (since those things are optional, like Krait).
If anything, to help both your points, I'd add an extra flag to say if these options are accepted as arguments/directives or not. So, internally, to find the flags, you'd pass "true" to the option of finding any, and front-ends/tools would pass "false", to only get what's parseable.
cheers,
--renato
Thanks for explaining the Krait situation - this now makes sense to me and I agree that it is not ideal.
If anything, to help both your points, I'd add an extra flag to say if these options are accepted as arguments/directives or not. So, internally, to find the flags, you'd pass "true" to the option of finding any, and front-ends/tools would pass "false", to only get what's parseable.
This seems like a good idea to me. That way we can control what is available from which direction. I think that the AArch32 +fp case for ARMv7 should be reason enough for us to want to do this. Saying -march=armv7-a+fp cannot give a sensible result, so restricting the available extns per backend is sensible.
Do we intend to resolve my other points regarding MP, Virtiualization (and to a lesser extent OS) in this patch?
With just two variants for the foreseeable future (A32/A64), I think it should be ok to have two flags.
Do we intend to resolve my other points regarding MP, Virtiualization (and to a lesser extent OS) in this patch?
Yes. Anything that deviates from either GCC/Darwin legacy or the ARM ARM must have an explicit and reasonable explanation.
I'd welcome more ARM folks having a thorough look at this, as you guys have a larger collective memory about these things. :)
Agreed - I can look at the next revision from Alexandros regarding adding MP and Virtualization to this table (I am less fussed about OS)
I am not sure I can review effectively the infrastructure part of limiting the extensions per frontend, but once that is in place, I am happy to review a new version of the arch extension table. I am also not clear whether that additional work should be part of this patch or can come later. Renato - what do you think?
Thanks!
I am not sure I can review effectively the infrastructure part of limiting the extensions per frontend, but once that is in place, I am happy to review a new version of the arch extension table. I am also not clear whether that additional work should be part of this patch or can come later. Renato - what do you think?
I think the best option would be to get the table correct with the current values, then extend it to a better model later.
- added more arch extensions
- separated hwdiv from default extensions in arm v7r, v7(e)m, v8a
Comments inline.
lib/Support/TargetParser.cpp | ||
---|---|---|
257 | Shouldn't this be "cortex-r5f" with another line for "cortex-r5" with no FP? Also, the Cortex-R5F FP unit is SP only. | |
303 | And this one is here because we want -mcpu=cortex-m4 to select Cortex-M4F rather than Cortex-M4? Why do we want that? Wouldn't we want it to match Cortex-R4? |
lib/Support/TargetParser.cpp | ||
---|---|---|
156 | IIUC - this field indicates whether the ARCHExtNames should be passed on as-is to the LLVM backend. Surely all of these flags should end up going to the backend somehow or another or I would not be enabling the backend features correctly? In this case, wouldn't a better field be a const char *backend_name ? Apologies if I have misunderstood what is happening here. | |
183 | I have now twigged what this is: Trustzone/Security Extensions. This is part of the base ARMv6Z architecture. Assuming we want to make the name context sensitive (which I think is a good idea) then there is a security extension as an optional extension on the ARMv7-A architecture profile and then becomes a base part of the ARMv8-A architecture. All of the ARMv7 Cortex-A* cores implement the security extensions. | |
230 | I don't know exactly how this all fits together, but should this not take the IWMMXT architecture extension mentioned in the above list? | |
231 | Again - there is an architectural extension for XSCALE which should presumably go here (although I don't know what you want it to do) | |
235 | This should be an ARMv6Z core. ARMv6Z is ARMv6 + Security extensions. | |
238 | This entry duplicates the two below it. This entry gets it wrong, so should go, the ARMv6SZ one is correct. | |
240 | Bin | |
294 | IIUC - all the entries here are intentional duplicates that are incorrect? |
lib/Support/TargetParser.cpp | ||
---|---|---|
156 | No, this is the "is it available to the parser" or not. As we discussed, this should be at least split into arm32 and arm64 flags. | |
183 | This is a legacy option from GCC, I believe. What it is now is what it was before (no one has changed it), so I wouldn't try to be smart here, at least not unless we understand the stakes. | |
240 | No, this is needed (sorry, it's a mess) to correctly identify that ARMv6ZK has a default CPU... | |
257 | possibly two lines, one for each. I guess we spoke about this before, but had no harder evidence that it was beneficial. Now, with the default options, it makes a clear cut to having both. | |
294 | these are just here to return the correct default CPU for the "unsupported" architecture IDs. With the other patch to use "generic" instead, this will be deprecated. Just let is stay for now. |
lib/Support/TargetParser.cpp | ||
---|---|---|
156 | OK - so a better name would do it then - CmdLineOptionAArch32, with CmdLineOptionAArch64 to follow once we hook the frontend into all this. | |
183 | Binutils docs confirm my interpretation: https://sourceware.org/binutils/docs/as/ARM-Options.html#ARM-Options | |
240 | In that case - should we move it to the "unsupported" section like the other bogus mappings? |
lib/Support/TargetParser.cpp | ||
---|---|---|
257 | I suggested this in a previous patch, however there is no logic for r5f in ARM.td or anywhere else I think. I also think r5f fpu is FK_VFPV3_D16. Let's double check. | |
303 | This looks like a typo of the previous patch. | |
357 | I added this function to distinguish base arch features from arch extension. Having more base features than just hwdiv (ex. security extensions, etc... as Richard mentioned to a previous comment) it would make sense to me either to create a new column to CPUNames table indicating base features (but the values will be ARM::AEK_* which is confusing) or add the base features to the existing column of DefaultExtensions. Let me know your thoughts on this. | |
464 | IsBackendOption is used here to decide whether to forward the features to ARMTargetInfo or not (if the backend does not recognize the features we have warnings and regressions). |
lib/Support/TargetParser.cpp | ||
---|---|---|
357 | *I guess the new column fits best in ARCHNames table rather than CPUNames table. |
r0 versions of Cortex-R5 did not have MP extensions. r1 versions do have the MP extensions. My understanding is that clang likes to target the latest revision for cores. If that understanding is correct, then it should have MP extensions.
lib/Support/TargetParser.cpp | ||
---|---|---|
464 | Sure - my point is that surely clang needs to forward all relevant extensions to the backend so it is set up correctly. So rather than only sending those with exactly the right name, surely a backend name field would be best so that all features could be sent to the backend? I am backseat driving here, so if this some other way that backend features get enabled, then feel free to ignore me. |
lib/Support/TargetParser.cpp | ||
---|---|---|
257 | If that is the case, then I am happy that it is outside the scope of your patch and can be left for another time. |
We tend to pick the most common, or what users expect they will see, not the latest.
People expect ARMv7A to have NEON, but I'm not sure people expect ARMv7M to have multiple cores.
However, the MP extension has little importance in the back-end, so I won't make much fuzz about it. Whatever you guys think it's best for the default is ok with me.
Maybe see what GCC does? In those unimportant cases, being consistent is always a better strategy. :)
lib/Support/TargetParser.cpp | ||
---|---|---|
183 | From that doc I infer that sec is v6K/v7A but NOT v8. | |
240 | Well, ok, let me explain what "supported" means. It wasn't a good idea to begin with, tbh. Supported == ARM describes it as an architecture, so the flag AK_ARMXXX makes sense in the back-end. The ones below "unsupported", AFAICT, make no difference over the "base" architecture, or are extensions that ARM knows nothing about, like ARMv7L. But the separation is not clear cut, and when we table-gen it, it will disappear, so I wouldn't worry about it. |
- Added the base features per architecture in ArchNames table.
- Added API to retrieve that info.
- Removed "arm1176j-s" from CPUNames table (invalid processor name).
- Added Cortex-r5f and m4f
- Added bool flags for representing 32/64-bit valid cmd options. However I am not sure they are correct and I recommend to remove these from this patch. They are not used anywhere.
include/llvm/Support/TargetParser.h | ||
---|---|---|
17–18 | Can you also change this comment to be something like: // FIXME: These includes are necessary for the target feature selection. Once those methods // move to TargetTuple, they should be removed. | |
184–185 | This is not directly related, but can you move these three methods further down, on their own? The comment to move them to TargetTuple applies to all three of them, but not to the ones below. | |
lib/Support/TargetParser.cpp | ||
90 | It's possible that these lines are passing the 80-column limits. Maybe align the build attributes to the others on a new line would fix that. | |
158 | You're not using those to parse depending on the arch. Maybe we should leave them for a separate patch, as this will have to change both ARM back-end and Clang users. | |
230 | ARM::AEK_IWMMXT | |
231 | ARM::AEK_XSCALE | |
235 | Don't remove it. I also saw that it was "invalid", but it was there because GCC accepts it, for whatever reason. If anything, deprecating this name will have to come on a different patch, after consensus. | |
235 | ARM::AEK_MP? ARM::AEK_SEC? | |
238 | Don't change the arch here. If this is wrong, should be discussed and fixed in a separate patch. | |
240 | Please, don't change *any* ARCH in this patch, nor remove *any* CPU name. | |
266 | You can't have two default CPUs for the same AK_ARCH | |
292–305 | ARM::AEK_IWMMXT | |
293 | ARM::AEK_XSCALE | |
358–359 | whitespace change doesn't belong | |
442 | this is for a different patch |
Agree with Renato's comments, apart from one.
lib/Support/TargetParser.cpp | ||
---|---|---|
183 | Well - the ARMv7-A security extension becomes mandatory in ARMv8-A AArch32. I guess the reason that they call out ARMv6Z and ARMv7-A separately here is that what constitutes "Security Extension" is different between those two arches, whereas in ARMv8-A it is the same as for ARMv7-A just mandatory. So if that is agreed, then I would expect AArch32 clang to accept this one. |
lib/Support/TargetParser.cpp | ||
---|---|---|
230 | Is this an extension or a base feature? If it is an extension, then it should be removed from base features. | |
231 | Is this an extension or a base feature? If it is an extension, then it should be removed from base features. | |
266 | oops | |
292–305 | Is this an extension or a base feature? If it is an extension, then it should be removed from base features. | |
293 | Is this an extension or a base feature? If it is an extension, then it should be removed from base features. | |
442 |
lib/Support/TargetParser.cpp | ||
---|---|---|
230 | We don't have *any* logic whatsoever for any of iwmmxt, xscale or maveric features. These are there just for compatibility with GCC, and to recognize the odd triples "xscale-apple-darwin" as ARM. So, I'm not too fussy about letting those ones be unused. What you could do is to add a comment on ArchExtNames just before them: // Unsupported and unused arch extensions | |
442 | Ok, makes sense. |
lib/Support/TargetParser.cpp | ||
---|---|---|
90 | Most of the tables of this file were already exceeding the 80c limit, but I think respecting this limit makes the tables hardly readable. Also, takes too much time to do this change. Is it worth? Do we really need to? |
lib/Support/TargetParser.cpp | ||
---|---|---|
90 | Unfortunately, yes. If they were already, that's my fault for not caring about it then, but that doesn't excuse me for not caring now. :) |
lib/Support/TargetParser.cpp | ||
---|---|---|
238 | that's what I mean, don't remove/change anything. The repercussions are not always obvious, not even on a clean check-all. |
lib/Support/TargetParser.cpp | ||
---|---|---|
235 | No, as far as I know. |
lib/Support/TargetParser.cpp | ||
---|---|---|
103 | I know it's a pain, but can you keep the flags in a separate line on their own if they're more than one? It's hard to follow when the flags are separate. Then build attributes always aligned to the others. | |
183 | Good point. | |
186 | Wait, if AEK_OS is unused or unsupported, why is it default for so many architectures? If we argue that IWXMMT, XSCALE and MAVERIC don't need to be applied, I'd argue that OS doesn't either. | |
247 | Same thing about alignment here | |
349 | Shouldn't this get the arch defaults and OR the values before return? | |
489 | You're not using this anywhere, I thought you would use above on getDefaultExtensions at least. Also, better name them accordingly. We're not yet separating extensions from features, and both are coming from the same place, but associated with a different lookup table, so the name is more than just misleading. getArchBaseExtensions() getCPUDefaultExtensions() would be better candidates, at least for now. |
lib/Support/TargetParser.cpp | ||
---|---|---|
186 | I will change that in the header file. AEK_OS was considered unsupported. I will change it's value so that it is not at the end of the 32-bit range. | |
349 | I don't understand this. DefaultExtensions is a bitmap that contains all the required information. What values should it OR? | |
489 | This will be used in ARMTargetInfo (see D11299) to get the base arch features. I am OK with renaming them accordingly. |
lib/Support/TargetParser.cpp | ||
---|---|---|
349 | OK I see what you mean. Yes I agree, I will change this. |
- Changed alignment in tables.
- Removed AEK_OS from unsupported range.
- getCPUDefaultExtensions() returns the OR of arch extensions and cpu extensions.
include/llvm/Support/TargetParser.h | ||
---|---|---|
138 | Er, AEK_OS *is* unsupported. In ARMAsmParser.cpp, structure "Extensions" states it clearly: // FIXME: Unsupported extensions. { ARM::AEK_OS, Feature_None, {} }, { ARM::AEK_IWMMXT, Feature_None, {} }, { ARM::AEK_IWMMXT2, Feature_None, {} }, { ARM::AEK_MAVERICK, Feature_None, {} }, { ARM::AEK_XSCALE, Feature_None, {} }, What I meant was to remove them from the Arch/CPU tables. | |
lib/Support/TargetParser.cpp | ||
81 | The usage of AEK_NONE here is weird. If we want NONE to mean "I know there are none", then it should have a specific value (like 0x1) and this should be checked against the other values, like: assert(Ext & AEK_NONE ? !(Ext & ~AEK_NONE) : true); This is the semantics I expected it to be when you assigned AEK_NONE = 0x1. But if we want NONE to mean "I don't know yet", so that we can OR it CPU/Arch values, like you're doing in getCPUDefaultExtensions(), than its value should be 0x0 and we don't need INVALID. | |
185 | Move this back down inside unsupported. | |
464 | So far, we have dealt with front vs back end by hiding the target-features in "setFeatures" functions, that I'd like to go somewhere else later. The problem is that "idiv" maps to either ARM or Thumb div, which tells nothing about SDIV vs UDIV. I don't particularly like the new options "hwdiv" or "hwdivarm", and they are indeed creating the problem of extending the target-options range to something we might not want to support in the future. I think we should drop that change for now, only keep what's currently supported by the assembler / front-end, and have this discussion at some other time. |
- removed "hwdiv" and "hwdiv-arm" from extensions table
- removed isBackendOption flag from extensions table
- moved AEK_OS to unsupported and removed it from all table entries
- removed all unsupported extensions from table entries
Hi Alexandros,
I'm afraid this part of the code has changed considerably and you'll have to re-base and probably will have a lot of work to get it back into shape.
cheers,
--renato
Hi Alexnadros,
Final round of comments, I think the patch is getting into shape to go.
Thanks,
--renato
include/llvm/Support/ARMTargetParser.def | ||
---|---|---|
217 ↗ | (On Diff #33933) | HWDIVARM on Cortex-R? Why AEK_MP here? (and not for the other Rs?) |
include/llvm/Support/ARMTargetParser.def | ||
---|---|---|
217 ↗ | (On Diff #33933) | It's the same for ARM divide. If you take a look at @richard.barton.arm 's first comment, these apply for cortex-r7 too. |
Can you also change this comment to be something like: