Page MenuHomePhabricator

[RISCV][MC] Add support for RV64E

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



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

Unit TestsFailed

60,080 msx64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::restore_stack.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=thread -Wall -m64 -msse4.2 -gline-tables-only -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -std=c++11 -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -O1 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp && not /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp 2>&1 | FileCheck /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp

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!


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


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

pcwang-thead added inline comments.Feb 8 2023, 7:24 PM

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

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

jrtc27 added inline comments.Feb 8 2023, 9:11 PM

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


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

pcwang-thead added inline comments.Feb 8 2023, 10:09 PM

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.EditedMon, Feb 20, 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.Mon, Feb 20, 3:54 AM

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