[RISCV] Support experimental/unratified extensions

[simoncook]

Summary

This adds support for enabling experimental/unratified RISC-V ISA
extensions in the -march string in the case where an explicit version
number has been declared, and the -menable-experimental-extensions flag
has been provided.

This follows the design as discussed on the mailing lists in the
following RFC: http://lists.llvm.org/pipermail/llvm-dev/2020-January/138364.html

Since the RISCV ToolChain definition currently rejects any extension
with an explicit version number, the parsing logic has been tweaked to
support this, and to allow standard extensions to have their versions
checked in future patches.

Support for the bitmanip 'b' extension has been added as a first example,
it should be clear how to extend this should vector 'v' land first.

[simoncook]

Don't put option in m_riscv_Features_Group, we don't want it being handled like a feature.

[rogfer01]

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

77

Suggestion: I think we can avoid these two functions (isExperimentalExtension and getExperimentalExtensionVersion) going out of sync if we have only one of them and make it return an llvm::Optional of the pair of versions.

Thent it can be used like this

if (auto ExperimentalExtension = isExperimentalExtension(Ext)) {
   std::pair<StringRef, StringRef> SupportedVers = *ExperimentalVersion;
   ...
}

I'd also add a comment that the pair's first is the major version and second is the minor version (or alternative use a struct with two public fields Major and Minor)

410

There is no test for that case but I'm afraid we can't test it yet, can we?

[simoncook]

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

77

That's a good idea, I'll update this to use this pattern.

410

We can test for e.g. rv32i_m which is now accepted (GCC also accepts this option we didn't before), but the version number code I don't think there is until we support versions on more than one extension.

[simoncook]

Switch to using Optional for returning version numbers.

[simoncook]

With the update to D69987 adding the Zvqmac predicate, it seems both the b and v extensions have Z extensions that also need supporting using this method, I'll update this to also support Z shortly, but don't expect the general method to change, just adding the same version check elsewhere.

[simoncook]

Add support for Z extensions also under this scheme.

In order to support these I've had to tweak the multi-letter extension parsing a little. The net result is that error messages printed regarding version numbers are now consistent between single letter standard extensions and any multi-letter extension.

This also makes a change to the feature names used for multi-letter extensions, previously an architecture like rv32isxx1_sxx2 would be accepted with the TargetFeatures sxx1 and sxx2 being added. Now an error indicating that sxx has been used multiple times in the string, with just the feature sxx being added.

[lewis-revill]

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

102–138

Typo 'number number' ? Also perhaps not accurate to say 'current version number', as opposed to something like 'a supported version number'.

103

Hmm... Not sure it makes sense anymore for the function to be called isExperimentalExtension, because it's not just returning true or false. Just off the top of my head I would call it something like getExperimentalExtensionVersion or ideally something more concise.

110

Is just testing Major.empty() not sufficient? We can't have a version with just a minor version number.

135

Ditto.

377

Should this comment be updated?

[simoncook]

Rebase, incorporate changes suggested by Lewis

[simoncook]

• Update to match latest dependencies
• Handle adding "experimental-" to SubtargetFeatures for experimental features

[lewis-revill]

Thanks, this is looking in good shape now. As long as everyone agrees on this scheme I think the implementation is good to go (pending the bitmanip extension support).

[lenary]

I'm happy with the approach here.

At some point we should put version numbers into the target features, but I think that can wait until we know how to cope with how the G extension expands into its other extensions (this has some open questions in general). This suggestion does not, in my mind, block this patch, as including experimental- in the target feature names does make it clear which features are experimental today.

[asb]

LGTM, thanks Simon!

[asb]

I've approved D65649 now, so I think this one can land as soon as that one does.