Page MenuHomePhabricator

[AArch64][AsmParser] Fix .arch_extension directive parsing
ClosedPublic

Authored by c-rhodes on Apr 2 2019, 3:31 AM.

Details

Summary

This patch fixes .arch_extension directive parsing to handle a wider
range of architecture extension options. The existing parser was parsing
extensions as an identifier which breaks for extensions containing a
"-", such as the "tlb-rmi" extension.

The extension is now parsed as a string. This is consistent with the
extension parsing in the .arch and .cpu directive parsing.

Diff Detail

Repository
rL LLVM

Event Timeline

c-rhodes created this revision.Apr 2 2019, 3:31 AM

The change looks good and straightforward to me, but I guess it could be good for someone else to have a look as well as I'm no authority here. I can mark it as approved in a few days if nobody else comments.

SjoerdMeijer added a comment.EditedApr 2 2019, 5:26 AM

This change looks also fine to me.
From a quick grep in the test directory, looks like we don't have many tests for the .arch_extension directive. I.e., I see only one, for the simd directive. I think that confirms my suspicion that we tend to forget to add tests/support for the .arch_extension directive when we upstream new architecture support. So, sorry to be a pain here, but while you're at it, my request would be to add tests for some other architecture extensions that we are missing. For example, rasv8_4 might be an interesting one because it has an underscore in it. Looks like the refactoring in D54633 introduced new extensions that we could test.

From a quick grep in the test directory, looks like we don't have many tests for the .arch_extension directive. I.e., I see only one, for the simd directive. I think that confirms my suspicion that we tend to forget to add tests/support for the .arch_extension directive when we upstream new architecture support. So, sorry to be a pain here, but while you're at it, my request would be to add tests for some other architecture extensions that we are missing. For example, rasv8_4 might be an interesting one because it has an underscore in it. Looks like the refactoring in D54633 introduced new extensions that we could test.

Hi Sjoerd, yes the tests are lacking for the .arch_extension directive, thanks for pointing that patch out I'll add a few tests for features with interesting characters in the name that are not currently covered.

@SjoerdMeijer It appears the .cpu, .arch and .arch_extension directives can only enable features defined in ExtensionMap in the AArch64AsmParser.cpp and ignores them otherwise. ExtensionMap seems incomplete, in the patch you mentioned only tlb-rmi, pan-rwv and ccpp features were added. There's also a comment:

2834     // FIXME: Unsupported extensions
2835     {"pan", {}},
2836     {"lor", {}},
2837     {"rdma", {}},
2838     {"profile", {}},

Which includes pan and lor which I don't understand why they are unsupported, perhaps they were added before the full feature support. I think additional work is required to improve ExtensionMap, I could add further tests for features currently available such as SVE in this patch, or is it also worth addressing the missing features?

It appears the .cpu, .arch and .arch_extension directives can only enable features defined in ExtensionMap in the AArch64AsmParser.cpp and ignores them otherwise. ExtensionMap seems incomplete, in the patch you mentioned only tlb-rmi, pan-rwv and ccpp features were added.

This whole ExtensionMap is a bit of a hack. It's a hack because it duplicates information already described in TargetParser (AArch64TargetParser.def). But this isn't an easy thing to fix I think.

I don't want to hold up this patch, but any low hanging fruit to make this more complete would be a really big bonus. Perhaps an approach is to first complete the tests with extensions already in ExtensionMap, that should hopefully just work. And if you feel strong about this, you can then add missing extension in a follow up patch.

c-rhodes updated this revision to Diff 193503.Apr 3 2019, 8:11 AM

Improved tests to cover all features supported by .arch_extension

@SjoerdMeijer I've added tests for all features currently in ExtensionMap.

SjoerdMeijer accepted this revision.Apr 3 2019, 8:34 AM

Nice one, thanks for fixing this!

This revision is now accepted and ready to land.Apr 3 2019, 8:34 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2019, 2:12 AM