Page MenuHomePhabricator

[RISCV][MC] Add support for RV64E
ClosedPublic

Authored by jobnoorman on Feb 8 2023, 3:23 AM.

Details

Summary

Implement MC support for the recently ratified RV64E base instruction set.

This patch does two things. First, it implements RV64E by trying to share as much code as possible with RV32E. This seems to make sense since the logic is exactly the same (restrict the available registers to x0-x15). However, it has some consequences that make me wonder if this is a good idea:

  • the existing feature (FeatureRV32E) and predicate (IsRV32E) have been renamed to FeatureRVE and IsRVE. The acronym "RVE" might not be immediately clear to people;
  • the RV128I base ISA doesn't currently define an equivalent RV128E variant. This is not a problem since LLVM doesn't implement it yet but we might have to reintroduce some checks this patch removes if it ever does.

Secondly, this patch introduces the recently proposed (but not yet ratified) lp64e ABI. It doesn't do anything besides setting the EF_RISCV_RVE ELF flag and some consistency checking. I'm not sure whether it's acceptable to introduce such an unstable ABI so I can remove this part or move it to a separate revision if that's preferred.

Diff Detail

Event Timeline

jobnoorman created this revision.Feb 8 2023, 3:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2023, 3:23 AM
jobnoorman requested review of this revision.Feb 8 2023, 3:23 AM
jobnoorman updated this revision to Diff 495808.Feb 8 2023, 5:09 AM

Fix Clang driver tests.

asb added a comment.EditedFeb 8 2023, 6:30 AM

Thanks Job - your comment "Secondly, this patch introduces the recently proposed (but not yet ratified) lp64e ABI. " gave me some concern that this patch may have included too many codegen changes in addition to the minimal MC layer needs, but delving into it I see that's not what you meant. The situation is a bit subtle.

  • ABI_LP64E is only used internally by LLVM and isn't directly exposed in the emitted ELF.
  • The EF_RISCV_RVE ELF attribute is set by this patch, but that flag is already defined in the ratified psABI doc and specified as "This bit is set when the binary targets the E ABI." So I think is correct.
  • EDIT: I'd originally worried about the stack alignment attribute, but further discussion and inspection of course confirms this attribute is only emitted to .s in the codegen path. RVE codegen isn't implemented yet, so the emission of this attribute won't happen - the codepath would probably be better off as an assert for now.
    • Relatedly, we don't seem to have any tests covering that attribute (i.e. I can emit ALIGN_16 for RV32E and ALIGN_4 for everything else and all our tests pass). It looks like fixing that would be a necessary step for this. (this comment still holds true - test coverage should be expanded here).

For anyone else following along and wondering how RVE escape the experimental extensions flag despite being only just ratified: RVE was one of the initial set of extensions that predated the whole ratification process, and at least some level of support shipped in GCC for quite some time.

(EDITED): I'd orriginally worried about the stack alignment question being a blocker, but upon fixing my understanding of the attribute emission that's no longer a concern (only affects the codegen path, which isn't implemented). So from my perspective, I think we'd be OK to integrate MC layer RV64E support (to the same level as RV32E) despite the lack of a finalised ABI. I'd welcome any other thoughts on this of course.

Other things this patch will eventually need are:

  • Updates to RISCVUsage.rst
  • Addition to the LLVM release notes llvm/docs/ReleaseNotes.rst (we're not always the best at this, but we try to add things as we go).

I'm wondering if @kito-cheng and @jrtc27 might be able to comment from the perspective of any GCC progress on RV64E support (Kito) and what's next for agreeing the RV64E ABI (both). Thanks in advance!

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
65

The TODO in the branch a few lines above should probably be copied here as well.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
49–50

Per the draft ABI this should be 8-byte alignment for RV64E.

pcwang-thead added inline comments.Feb 8 2023, 7:24 PM
llvm/lib/Target/RISCV/RISCVFeatures.td
499

How about this?

def IsRVE : Predicate<"Subtarget->isRVE()">,
                        AssemblerPredicate<(all_of FeatureRVE)>;
def IsRV32E : Predicate<"Subtarget->is32RVE()">,
                        AssemblerPredicate<(all_of FeatureRVE, Feature32Bit)>;
def IsRV64E : Predicate<"Subtarget->is64RVE()">,
                       AssemblerPredicate<(all_of FeatureRVE, Feature64Bit)>;
pcwang-thead added inline comments.Feb 8 2023, 7:30 PM
llvm/lib/Target/RISCV/RISCVFeatures.td
499

Oops, I mean Subtarget->isRV32E() and Subtarget->isRV64E().

jrtc27 added inline comments.Feb 8 2023, 9:11 PM
llvm/lib/Target/RISCV/RISCVFeatures.td
499

Why do you need that? Just use IsRV32/64 and IsRVE as appropriate. We don't have IsRV32F, IsRV64D, IsRV32A, etc.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
77–80

They'll surely come together, just change the error message to RVE?

pcwang-thead added inline comments.Feb 8 2023, 10:09 PM
llvm/lib/Target/RISCV/RISCVFeatures.td
499

I have no strong opinion on this, it's just a suggestion on one of the concerns in the patch description:

the existing feature (FeatureRV32E) and predicate (IsRV32E) have been renamed to FeatureRVE and IsRVE. The acronym "RVE" might not be immediately clear to people;

And we do have RV32E and RV64E in the spec, maybe we should rename them to RVE?

jobnoorman updated this revision to Diff 496046.Feb 9 2023, 1:55 AM

Address reviewer comments:

  • Sidestep stack alignment uncertainty by using report_fatal_error when codegen is reached for RVE;
  • Unify codegen error reporting for RV32E and RV64E;
  • Update RISCVUsage.rst and ReleaseNotes.rst
jobnoorman marked 3 inline comments as done.Feb 9 2023, 1:57 AM
jobnoorman updated this revision to Diff 496049.Feb 9 2023, 2:01 AM

Fix patch apply error. Sorry about the noise.

asb accepted this revision.EditedFeb 20 2023, 3:54 AM

Everything that's in here looks good to me, and I think all the review comments have been addressed.

One thing I've just spotted is we still mark the E extension as 1.9, while the ratified version appears to be 2.0. I recently added a test for __riscv_i in riscv-target-features.c, but it's not possible to add the analogous one without more codegen support (the clang invocation errors out due to the ilp32e abi being selected). We're probably missing rv32e coverage here. Most extension versions are reflected in attributes.ll, but of course we'll error out due to codegen. EDIT: probably belongs in attribute-arch.s.

I personally think just bumping E from 1.9 to 2.0 shouldn't be a big dea., but it's perhaps something that might be best not to mingle in this patch - as noted before, RVE was defined long before the ratification process exists, so it is a bit of a special case. How posting a separate patch that moves E from 1.9 to 2.0 (which would be fine to apply even before adding RV64E), and ensuring we have some test coverage for that change? Thanks!

This revision is now accepted and ready to land.Feb 20 2023, 3:54 AM

Allow rv64e in arch strings (e.g., in the arch assembly attribute). Also add a test for this.

kito-cheng accepted this revision.Mar 23 2023, 2:34 AM

@jobnoorman do you need some help to commit this and D144384? let me know if you need help, and I would suggest you could drop a mail to Chris to obtain write access https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access :)

@jobnoorman do you need some help to commit this and D144384? let me know if you need help, and I would suggest you could drop a mail to Chris to obtain write access https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access :)

If you could commit these, that would be very helpful :) I was planning to ask for commit access after landing these.

Author details: Job Noorman <jnoorman@igalia.com>

@jobnoorman could you rebase again? I got conflict during apply this patch

asb added a comment.Mar 23 2023, 3:46 AM

I expect you'll need to / want to update the C++ RISCVISAInfo unit tests as they were added after this patch was first proposed.

Woops, sorry about that. Done!

I expect you'll need to / want to update the C++ RISCVISAInfo unit tests as they were added after this patch was first proposed.

My previous comment was about the rebase, I will look into this now.

  • Add RISCVISAInfo unit tests
  • Update error message of RISCVISAInfo::parseArchString to reflect that we support RV64E now
asb accepted this revision.Mar 23 2023, 5:14 AM

LGTM - thanks. I'll land this for you now since it already got a LGTM from Kito.

Fix failing test, sorry about that!

This revision was landed with ongoing or failed builds.Mar 23 2023, 5:33 AM
Closed by commit rGc39dd7c1db97: [RISCV][MC] Add support for RV64E (authored by jobnoorman, committed by asb). · Explain Why
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 5:33 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
asb added a comment.Mar 23 2023, 5:51 AM

Apologies, the commited version of this patch missed the clang/test/Driver/riscv-arch.c change. rGe54cdd058e223bd62840e901b8b462c011d2fae5 committed to fix this.