[PATCH] D143570: [RISCV][MC] Add support for RV64E

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

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` 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.
- So far so good. The bit that's more tricky is the stack alignment attribute. Your patch doesn't match the proposed ABI (8 bytes) while it probably should, and we now get into the territory of generating binaries with attributes that might prove to be incorrect if the psABI discussion doesn't result in agreeing an 8 byte stack alignment.
  - 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.

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.

>From my perspective, we're almost able to integrate MC layer RV64E support (providing the same level of "support" as for RV32E) without worrying about the ABI not yet being finalised, but the only blocker is the question about the stack alignment.

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!



================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp:65
+             TargetABI != ABI_Unknown) {
+    errs()
+        << "Only the lp64e ABI is supported for RV64E (ignoring target-abi)\n";
----------------
The TODO in the branch a few lines above should probably be copied here as well.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:50
 void RISCVTargetStreamer::emitTargetAttributes(const MCSubtargetInfo &STI) {
-  if (STI.hasFeature(RISCV::FeatureRV32E))
+  if (!STI.hasFeature(RISCV::Feature64Bit) && STI.hasFeature(RISCV::FeatureRVE))
     emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_4);
----------------
Per the draft ABI this should be 8-byte alignment for RV64E.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143570/new/

https://reviews.llvm.org/D143570