This is an archive of the discontinued LLVM Phabricator instance.

Arch extensions and default target features in TargetParser
ClosedPublic

Authored by labrinea on Jul 29 2015, 5:37 AM.

Details

Summary
  • Added arch extensions in TargetParser
  • Added API for retrieving the corresponding target features
  • To be tested by clang

Diff Detail

Event Timeline

labrinea updated this revision to Diff 30897.Jul 29 2015, 5:37 AM
labrinea retitled this revision from to API for retrieving default FPU target features from TargetParser.
labrinea updated this object.
labrinea added inline comments.Jul 29 2015, 6:11 AM
lib/Support/TargetParser.cpp
372

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.
rengolin edited edge metadata.Jul 30 2015, 4:19 AM

I have a few concerns about the direction we are taking here which can be summarised as:

  • Do we care about GCC compatibility?

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?

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.

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?

Agreed - I can look at the next revision from Alexandros regarding adding MP and Virtualization to this table (I am less fussed about OS)

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.

Am I right that ARM::AEK_SIMD means NEON extension and not SIMD32 instructions?

labrinea updated this revision to Diff 31043.Jul 30 2015, 10:23 AM
labrinea retitled this revision from API for retrieving default FPU target features from TargetParser to API for retrieving default target features from TargetParser.
labrinea edited edge metadata.
  • added more arch extensions
  • separated hwdiv from default extensions in arm v7r, v7(e)m, v8a

Comments inline.

lib/Support/TargetParser.cpp
261

Shouldn't this be "cortex-r5f" with another line for "cortex-r5" with no FP?

Also, the Cortex-R5F FP unit is SP only.

307

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.

169

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.

234

I don't know exactly how this all fits together, but should this not take the IWMMXT architecture extension mentioned in the above list?

235

Again - there is an architectural extension for XSCALE which should presumably go here (although I don't know what you want it to do)

239

This should be an ARMv6Z core. ARMv6Z is ARMv6 + Security extensions.

242

This entry duplicates the two below it. This entry gets it wrong, so should go, the ARMv6SZ one is correct.

244

Bin

298

IIUC - all the entries here are intentional duplicates that are incorrect?

rengolin added inline comments.Jul 30 2015, 1:31 PM
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.

169

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.

244

No, this is needed (sorry, it's a mess) to correctly identify that ARMv6ZK has a default CPU...

261

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.

298

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.

169
244

In that case - should we move it to the "unsupported" section like the other bogus mappings?

labrinea added inline comments.Jul 31 2015, 1:59 AM
lib/Support/TargetParser.cpp
261

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.

307

This looks like a typo of the previous patch.

361

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.

468

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

labrinea added inline comments.Jul 31 2015, 2:06 AM
lib/Support/TargetParser.cpp
361

*I guess the new column fits best in ARCHNames table rather than CPUNames table.

Regarding the table, apart from the above Divide concern, I notice also

  • the lack of MP extension for Cortex-R5

Are we sure Cortex-R5 has MP extensions?

Regarding the table, apart from the above Divide concern, I notice also

  • the lack of MP extension for Cortex-R5

Are we sure Cortex-R5 has MP extensions?

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
468

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
261

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.

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.

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
169

From that doc I infer that sec is v6K/v7A but NOT v8.

244

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.

labrinea updated this revision to Diff 31123.EditedJul 31 2015, 7:51 AM
  • 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.
rengolin added inline comments.Jul 31 2015, 9:04 AM
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.
185–186

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.

234

ARM::AEK_IWMMXT

235

ARM::AEK_XSCALE

239

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.

239

ARM::AEK_MP? ARM::AEK_SEC?

242

Don't change the arch here. If this is wrong, should be discussed and fixed in a separate patch.

244

Please, don't change *any* ARCH in this patch, nor remove *any* CPU name.

270

You can't have two default CPUs for the same AK_ARCH

296–309

ARM::AEK_IWMMXT

297

ARM::AEK_XSCALE

362–363

whitespace change doesn't belong

446

this is for a different patch

Agree with Renato's comments, apart from one.

lib/Support/TargetParser.cpp
169

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.

labrinea added inline comments.Jul 31 2015, 9:20 AM
lib/Support/TargetParser.cpp
234

Is this an extension or a base feature? If it is an extension, then it should be removed from base features.

235

Is this an extension or a base feature? If it is an extension, then it should be removed from base features.

270

oops

296–309

Is this an extension or a base feature? If it is an extension, then it should be removed from base features.

297

Is this an extension or a base feature? If it is an extension, then it should be removed from base features.

446

This is required by D11299 which was split to D11590. Could we avoid the bureaucracy please?

rengolin added inline comments.Jul 31 2015, 9:32 AM
lib/Support/TargetParser.cpp
234

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
446

Ok, makes sense.

labrinea added inline comments.Jul 31 2015, 9:38 AM
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?

rengolin added inline comments.Jul 31 2015, 9:41 AM
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. :)

labrinea added inline comments.Jul 31 2015, 10:00 AM
lib/Support/TargetParser.cpp
242

It is not changed. "arm1176j-s" was removed and everything went up one line ;)

244

everything went up one line

rengolin added inline comments.Jul 31 2015, 10:01 AM
lib/Support/TargetParser.cpp
242

that's what I mean, don't remove/change anything. The repercussions are not always obvious, not even on a clean check-all.

labrinea added inline comments.Jul 31 2015, 10:20 AM
lib/Support/TargetParser.cpp
239

No, as far as I know.

labrinea updated this revision to Diff 31130.Jul 31 2015, 10:26 AM

Updated according to previous comments.

rengolin added inline comments.Jul 31 2015, 11:07 AM
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.

169

Good point.

172

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.

251

Same thing about alignment here

353

Shouldn't this get the arch defaults and OR the values before return?

493

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.

labrinea added inline comments.Aug 1 2015, 2:48 AM
lib/Support/TargetParser.cpp
172

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.

353

I don't understand this. DefaultExtensions is a bitmap that contains all the required information. What values should it OR?

493

This will be used in ARMTargetInfo (see D11299) to get the base arch features. I am OK with renaming them accordingly.

labrinea added inline comments.Aug 1 2015, 1:38 PM
lib/Support/TargetParser.cpp
353

OK I see what you mean. Yes I agree, I will change this.

labrinea updated this revision to Diff 31195.Aug 1 2015, 4:16 PM
  • Changed alignment in tables.
  • Removed AEK_OS from unsupported range.
  • getCPUDefaultExtensions() returns the OR of arch extensions and cpu extensions.
rengolin added inline comments.Aug 3 2015, 2:26 AM
include/llvm/Support/TargetParser.h
139

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.

171

Move this back down inside unsupported.

468

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.

labrinea updated this revision to Diff 32690.Aug 20 2015, 6:43 AM
  • 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

labrinea updated this revision to Diff 33933.Sep 3 2015, 5:21 AM
labrinea retitled this revision from API for retrieving default target features from TargetParser to Arch extensions and default target features in TargetParser.
labrinea updated this object.

Rebased

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

labrinea added inline comments.Sep 4 2015, 9:04 AM
include/llvm/Support/ARMTargetParser.def
217 ↗(On Diff #33933)

Regarding the table, apart from the above Divide concern, I notice also

  • the lack of MP extension for Cortex-R5

Are we sure Cortex-R5 has MP extensions?

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.

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.

rengolin accepted this revision.Sep 4 2015, 9:23 AM
rengolin edited edge metadata.

Right, ok. LGTM. Thanks!

This revision is now accepted and ready to land.Sep 4 2015, 9:23 AM
labrinea closed this revision.Oct 9 2015, 3:55 AM

committed as r246930