This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support experimental/unratified extensions
ClosedPublic

Authored by simoncook on Feb 3 2020, 6:38 AM.

Details

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.

Diff Detail

Event Timeline

simoncook created this revision.Feb 3 2020, 6:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2020, 6:38 AM
simoncook updated this revision to Diff 242080.Feb 3 2020, 8:04 AM

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

rogfer01 added inline comments.Feb 4 2020, 12:55 AM
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)

407

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

simoncook marked 2 inline comments as done.Feb 4 2020, 2:46 AM
simoncook added inline comments.
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
77

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

407

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 updated this revision to Diff 242286.Feb 4 2020, 4:04 AM

Switch to using Optional for returning version numbers.

khchen added a subscriber: khchen.Feb 5 2020, 7:45 PM
simoncook planned changes to this revision.EditedFeb 7 2020, 2:44 AM

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 updated this revision to Diff 243325.Feb 7 2020, 5:08 PM

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 added inline comments.
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
102

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.

374

Should this comment be updated?

simoncook updated this revision to Diff 243502.Feb 10 2020, 3:30 AM

Rebase, incorporate changes suggested by Lewis

simoncook updated this revision to Diff 250770.Mar 17 2020, 7:42 AM
  • Update to match latest dependencies
  • Handle adding "experimental-" to SubtargetFeatures for experimental features
lewis-revill accepted this revision.Mar 18 2020, 2:24 AM

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

This revision is now accepted and ready to land.Mar 18 2020, 2:24 AM
lenary accepted this revision.Mar 19 2020, 4:37 AM

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 accepted this revision.Mar 19 2020, 6:27 AM

LGTM, thanks Simon!

asb added a comment.Apr 9 2020, 7:40 AM

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

This revision was automatically updated to reflect the committed changes.