This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] CodeGen of RVE and ilp32e/lp64e ABIs
ClosedPublic

Authored by wangpc on Nov 18 2019, 8:34 AM.

Details

Summary

This commit includes the necessary changes to clang and LLVM to support
codegen of RVE and the ilp32e/lp64e ABIs.

The differences between RVE and RVI are:

  • RVE reduces the integer register count to 16(x0-x16).
  • The ABI should be ilp32e for 32 bits and lp64e for 64 bits.

RVE can be combined with all current standard extensions.

The central changes in ilp32e/lp64e ABI, compared to ilp32/lp64 are:

  • Only 6 integer argument registers (rather than 8).
  • Only 2 callee-saved registers (rather than 12).
  • A Stack Alignment of 32bits (rather than 128bits).
  • ilp32e isn't compatible with D ISA extension.

If ilp32e or lp64 is used with an ISA that has any of the registers
x16-x31 and f0-f31, then these registers are considered temporaries.

To be compatible with the implementation of ilp32e in GCC, we don't use
aligned registers to pass variadic arguments and set stack alignment\
to 4-bytes for types with length of 2*XLEN.

FastCC is also supported on RVE, while GHC isn't since there is only one
avaiable register.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Provide correct datalayout
  • Add clang support and tests
  • Modify fp reservation code so we don't always use a frame pointer, even if we conservatively reserve it when using ilp32e + D extension.
jrtc27 added inline comments.Jan 14 2021, 4:08 PM
clang/lib/CodeGen/TargetInfo.cpp
10323 โ†—(On Diff #316800)

I think it'd be better to have a NumArgGPRs(EAABI ? 6 : 8) here as having a default value that gets overwritten is more error-prone (and harder to follow).

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
17040

Underscores with camel-case isn't great. Maybe ArgIGPRs and ArgEGPRs or similar?

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
116

Shouldn't this all be done by the generic stack realignment code like any other allocation? Or is the issue because it's _register spills_ not explicit allocas?

llvm/test/CodeGen/RISCV/stack-realignment.ll
3โ€“5

Multiple prefixes is a bad idea with update_llc_test_checks.py, and why is this one done differently from the rest?

lenary updated this revision to Diff 316806.Jan 14 2021, 4:21 PM
lenary marked an inline comment as done.
  • Add Variadic Testcase
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
17247

I missed that I need to cover this case.

I'm going to upload a testcase based on your example, but I'm not quite convinced it's correct. It does seem to align the stack correctly for the fp64, but that's maybe not the right thing to be doing here?

I haven't managed to execute the assembly in the testcase, but I thought adding the testcase was important.

llvm/test/CodeGen/RISCV/callee-saved-fpr64s.ll
14

I went back and thinking about this, we just need to make sure fp is reserved for later, rather than overriding hasFP, so we don't need to reserve FP unnecessarily.

Iterating over used registers to find FP64 registers didn't fill me with joy, and if you override canRealignStackFrame, it seems you just get very incorrect stack management (where the code justโ€ฆ doesn't bother to realign the stack before saving/restoring).

rkruppe removed a subscriber: rkruppe.Jan 15 2021, 12:56 AM
lenary edited the summary of this revision. (Show Details)Jan 15 2021, 1:24 AM
lenary added a reviewer: jrtc27.
lenary updated this revision to Diff 316875.Jan 15 2021, 1:43 AM
lenary marked an inline comment as done.

Address @jrtc27's Feedback:

  • Cleaned up Clang's RISC-V ABI Lowering Code
  • Cleaned up tests
  • Cleaned up leftover method implementation
  • Updated fp reservation comment
  • Cleaned up names
lenary marked 3 inline comments as done.Jan 15 2021, 1:51 AM
lenary added inline comments.
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
116

Yeah the issue is because itโ€™s register spills. I have a nice long commit message I wrote that I should update the summary with.

Comment updated nonetheless

llvm/lib/Target/RISCV/RISCVRegisterInfo.h
40 โ†—(On Diff #316806)

I forgot this was left in after some experimentation I did. Will remove it in the next update.

llvm/test/CodeGen/RISCV/callee-saved-fpr32s.ll
28

These check lines are left over from before. will remove

llvm/test/CodeGen/RISCV/stack-realignment.ll
3โ€“5

It also doesnโ€™t help to avoid duplication here.

khchen added a comment.May 3 2021, 2:25 AM

Hi, I would like to add ilp32e ABI support in llvm
Is there anyone working on this?
It seem the one thing missed is ilp32e ABI should disallow D ISA extension.
Is there anything else?

asb added a comment.May 12 2021, 9:36 AM

Hi, I would like to add ilp32e ABI support in llvm
Is there anyone working on this?
It seem the one thing missed is ilp32e ABI should disallow D ISA extension.
Is there anything else?

Nobody is currently working on this on the lowRISC side, so please do pick this up if you're interested. I can't quite recall the open issues I'm afraid.

Hi, all. Why is it not continued?

Hi, all. Why is it not continued?

Sorry, I have to work on other tasks so stop the rv32e implementation work.
Are you interest to finish it? I could share my patches to you.

Hi, all. Why is it not continued?

Sorry, I have to work on other tasks so stop the rv32e implementation work.
Are you interest to finish it? I could share my patches to you.

Is it (D70401) good enough to solve or complete rv32e issue?

khchen added a comment.EditedDec 6 2021, 6:27 PM

Is it (D70401) good enough to solve or complete rv32e issue?

It need to

  1. disallow ilp32e ABI with D ISA extension. https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/3f81fae0412bb9ad4002a4ade508be7aa5e1599b/riscv-cc.adoc#ilp32e-calling-convention
  2. emit predefined marco in header (__riscv_e)
  3. markSuperRegs for X16-X31
  4. update tests after rebase on main.

Is it (D70401) good enough to solve or complete rv32e issue?

It need to

  1. disallow ilp32e ABI with D ISA extension. https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/3f81fae0412bb9ad4002a4ade508be7aa5e1599b/riscv-cc.adoc#ilp32e-calling-convention
  2. emit predefined marco in header (__riscv_e)
  3. markSuperRegs for X16-X31
  4. update tests after rebase on main.

Nice. If no body objects, @pcwang-thead will take this task and re-draft a review.

lenary marked an inline comment as done.Dec 13 2021, 1:49 AM

Is it (D70401) good enough to solve or complete rv32e issue?

It need to

  1. disallow ilp32e ABI with D ISA extension. https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/3f81fae0412bb9ad4002a4ade508be7aa5e1599b/riscv-cc.adoc#ilp32e-calling-convention
  2. emit predefined marco in header (__riscv_e)
  3. markSuperRegs for X16-X31
  4. update tests after rebase on main.

Nice. If no body objects, @pcwang-thead will take this task and re-draft a review.

Please do feel free to commandeer the current patch. I cannot continue to work on it (so, you won't see review comments from me).

โ€ข pcwang-thead commandeered this revision.Dec 15 2021, 11:46 PM
โ€ข pcwang-thead added a reviewer: lenary.
  • Rebase.
  • Reserve x16-x31 when using RV32E.
  • Make ilp32e incompatible with D extension.
  • Update/Add tests.
โ€ข pcwang-thead retitled this revision from [WIP][RISCV] Implement ilp32e ABI to [WIP][RISCV] Complete RV32E/ilp32e implementation.Dec 16 2021, 12:18 AM
โ€ข pcwang-thead edited the summary of this revision. (Show Details)
  • Add macro __riscv_32e.
โ€ข pcwang-thead edited the summary of this revision. (Show Details)Dec 16 2021, 1:30 AM
โ€ข pcwang-thead edited the summary of this revision. (Show Details)
โ€ข pcwang-thead retitled this revision from [WIP][RISCV] Complete RV32E/ilp32e implementation to [RISCV] Complete RV32E/ilp32e implementation.Dec 27 2021, 10:10 PM
lenary resigned from this revision.Jan 4 2022, 3:14 AM

Gentle ping.

We are testing this patch and I'd like to get some nice advice.

  1. please add a check here and a clang cc1 test for it.
  2. Have you try to run llvm-test-suite with rv32e config on qemu?
llvm/lib/Support/TargetParser.cpp
339 โ†—(On Diff #396384)

why do we need to change the order?

llvm/lib/Support/TargetParser.cpp
339 โ†—(On Diff #396384)

IMO, when e is combined with d, e should have higher priority so that the default ABI will be ilp32e and then this error will be reported.

zixuan-wu commandeered this revision.Feb 15 2022, 11:43 PM
zixuan-wu added a reviewer: โ€ข pcwang-thead.
This comment was removed by zixuan-wu.

It's difficult to run llvm-test-suite in ilp32e abi in Linux. Because there are no workable environment such as runtime and kernel for ilp32e in GNU series tools.
And we can not run llvm-test-suite in baremental environment(NOT linux but elf triple). So I have a question about how to test llvm in elf triple and environment? Is there any test case llvm community normally uses and accepts?

Sorry for the wrong action of commandeer, @pcwang-thead will still be the author.

It's difficult to run llvm-test-suite in ilp32e abi in Linux. Because there are no workable environment such as runtime and kernel for ilp32e in GNU series tools.
And we can not run llvm-test-suite in baremental environment(NOT linux but elf triple). So I have a question about how to test llvm in elf triple and environment? Is there any test case llvm community normally uses and accepts?

I believe you can try QEMU, I tried it before.
https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/JE0aG-Mr0u4/m/tfFoITv7AgAJ
For llvm-test-suite, you could disable some non-baremental tests manually.
I found some issues in my local rv32e implementation by running llvm-test-suite before, it's why I think it's good to have a test.

Maybe the other reviewers have different opinions about this.
@luismarques @jrtc27 @asb @kito-cheng What do you think?

โ€ข pcwang-thead commandeered this revision.EditedMar 15 2022, 8:34 PM
โ€ข pcwang-thead edited reviewers, added: zixuan-wu; removed: โ€ข pcwang-thead.
  1. please add a check here and a clang cc1 test for it.
  2. Have you try to run llvm-test-suite with rv32e config on qemu?
  1. Thanks, I may do it later. And here is a question: the comment says It is illegal to specify 'e' extensions with 'f' and 'd'.

While ilp32e says:

The ILP32E calling convention is not compatible with ISAs that have registers that require load and store alignments of more than 32 bits. In particular, this calling convention must not be used with the D ISA extension.

And, the RV32E chapter in RISCV ISA manual says:

RV32E can be combined with all current standard extensions.

If I understand correctly, E can't be combined with D in current specification since E must use ILP32E calling convention.

  1. I have run llvm-test-suite with rv32e on qemu, and found no major fault for current implementation. Some tests are disabled because they can't run on bare mental (sees Disabled llvm-test-suite cases).

There are some failed tests due to floating-point precision, but I saw the same result when run with rv32imc on bare mental. I haven't taken the time to find out the reason, but I guess it may be soft-float issues.

Herald added a project: Restricted Project. ยท View Herald TranscriptMar 15 2022, 8:34 PM
Herald added a subscriber: arichardson. ยท View Herald Transcript
  1. please add a check here and a clang cc1 test for it.
  2. Have you try to run llvm-test-suite with rv32e config on qemu?
  1. Thanks, I may do it later. And here is a question: the comment says It is illegal to specify 'e' extensions with 'f' and 'd'.

While ilp32e says:

The ILP32E calling convention is not compatible with ISAs that have registers that require load and store alignments of more than 32 bits. In particular, this calling convention must not be used with the D ISA extension.

And, the RV32E chapter in RISCV ISA manual says:

RV32E can be combined with all current standard extensions.

If I understand correctly, E can't be combined with D in current specification since E must use ILP32E calling convention.

IMO, at least clang need to follows the gcc's implementation.
I guess gcc implementation follow riscv-elf-psabi-doc, @kito-cheng could you please confirm that?

  1. I have run llvm-test-suite with rv32e on qemu, and found no major fault for current implementation. Some tests are disabled because they can't run on bare mental (sees Disabled llvm-test-suite cases).

There are some failed tests due to floating-point precision, but I saw the same result when run with rv32imc on bare mental. I haven't taken the time to find out the reason, but I guess it may be soft-float issues.

Thanks for testing!! I also tested your patch locally,
Could you please make sure all gcc and clang results are the same in your failed tests?

I found https://github.com/llvm/llvm-test-suite/blob/main/SingleSource/UnitTests/2003-05-26-Shorts.c result is mismatched with gcc's (-march=rv32e -mabi=ilp32e).
Did you have same issue?

my build option:

$/path/to/rv32e-gcc/bin/riscv32-unknown-elf-gcc -march=rv32e -mabi=ilp32e 2003-05-26-Shorts.c
$./bin/clang --target=riscv32 -march=rv32e -mabi=ilp32e --gcc-toolchain=/path/to/rv32e-gcc/ 2003-05-26-Shorts.c

clang output:

   ui = 3318069411 (0xc5c5b8a3)         UL-ui = 0 (0xafafafaf)                            
ui*ui = 2382936009 (0x8e08b7c9)   UL/ui = -2060025877491592863 (0xe369516100000000)       
                                                                                          
    i = -976897885 (0xc5c5b8a3) L-i = 0 (0xafafafb0)                                      
 i* i = -1912031287 (0x8e08b7c9)        L/ i = 6996953267980741613 (0x611a2bed00000001)   
                                                                                          
us    = 47267 (0xb8a3)          UL-us = -4195947477825748992 (0xc5c50000afafafaf)         
us*us = 2234169289 (0x852ab7c9)   UL/us = 1452874783539635691 (0x1429a5eb0000f397)        
                                                                                          
 s    = -18269 (0xffffb8a3)     L-s = -4195666002849038335 (0xc5c60000afafafaf)           
 s* s = 333756361 (0x13e4b7c9)  L/ s = -7718140893307295808 (0x94e3a7c00001201b)          
                                                                                          
ub    = 163 (0xa3)              UL-ub = -4195745167686238208 (0xc5c5b800afafafaf)         
ub*ub = 26569 (0x67c9)          UL/ub = 2350833624863004346 (0x209fd6ba0113eca9)          
                                                                                          
 b    = -93 (0xffffffa3)                L-b = -4195744068174610431 (0xc5c5b900afafafaf)   
 b* b = 8649 (0x21c9)                   L/b = -1938405340110362979 (0xe519669d00dd1421)

gcc output:

   ui = 3318069411 (0xc5c5b8a3)         UL-ui = -5787213829993660416 (0xafafafaf00000000)
ui*ui = 2382936009 (0x8e08b7c9)   UL/ui = 3815330145 (0xe3695161)

    i = -976897885 (0xc5c5b8a3) L-i = -5787213825698693120 (0xafafafb000000000)
 i* i = -1912031287 (0x8e08b7c9)        L/ i = 5924072429 (0x1611a2bed)

us    = 47267 (0xb8a3)          UL-us = -5787213826675638272 (0xafafafafc5c50000)
us*us = 2234169289 (0x852ab7c9)   UL/us = 267830203885035 (0xf3971429a5eb)

 s    = -18269 (0xffffb8a3)     L-s = -5787213826675572736 (0xafafafafc5c60000)
 s* s = 333756361 (0x13e4b7c9)  L/ s = 316777810864064 (0x1201b94e3a7c0)

ub    = 163 (0xa3)              UL-ub = -5787213826675591168 (0xafafafafc5c5b800)
ub*ub = 26569 (0x67c9)          UL/ub = 77665829736404666 (0x113eca9209fd6ba)

 b    = -93 (0xffffffa3)                L-b = -5787213826675590912 (0xafafafafc5c5b900)
 b* b = 8649 (0x21c9)                   L/b = 62228105663178397 (0xdd1421e519669d)

If I understand correctly, E can't be combined with D in current specification since E must use ILP32E calling convention.

Calling convention and extensions are separated, calling convention are specify the how argument passing and the register convention, so ILP32E *can* use with -march=rv32efd, but it can't pass or return floating point type in FPR.

Just like we can ILP32 for -march=rv32ifd and LP64 with -march=rv64ifd, you may confused about the opposite combination like ILP32D with -march=rv32i and LP64D with -march=rv64i is not work, that's because it require pass or return floating point type in FPR, but FPR isn't existing in such ISA config.

Last LLVM sync-up call @asb has raise the discussion about the ILP32E issue, so here is note from my site:

RISC-V psABI doc still say "we don't guarantee the stability of ILP32E", the reason is RV32E still not a ratified extension, but as psABI chair, what I can say is we intend to do NOT change as possible.

As I know rv32e*/ilp32e are already used by many vendors (include SiFive), so I support ilp32e should be supported on LLVM upstream.

Last LLVM sync-up call @asb has raise the discussion about the ILP32E issue, so here is note from my site:

RISC-V psABI doc still say "we don't guarantee the stability of ILP32E", the reason is RV32E still not a ratified extension, but as psABI chair, what I can say is we intend to do NOT change as possible.

As I know rv32e*/ilp32e are already used by many vendors (include SiFive), so I support ilp32e should be supported on LLVM upstream.

Thanks! I will spend some time to make this patch compatible with GCC implementation, please feel free to give some comments and suggestions!

โ€ข pcwang-thead added a comment.EditedMar 23 2022, 1:50 AM

I found https://github.com/llvm/llvm-test-suite/blob/main/SingleSource/UnitTests/2003-05-26-Shorts.c result is mismatched with gcc's (-march=rv32e -mabi=ilp32e).
Did you have same issue?

I got the same issue, but it may be not this patch's problem.
Here is the reduced case:

#include <stdio.h>

unsigned long long getL() { return 0xafafafafc5c5b8a3ull; }
int main(int argc, char **argv) {
  unsigned long long UL = getL();     /* 0xafafafafc5c5b8a3 */
  unsigned int ui = (unsigned int)UL; /* 0xc5c5b8a3 =  3318069411 */
  printf("ui = %u (0x%x)\t\tUL-ui = %lld (0x%llx)\n", ui, ui, UL - ui, UL - ui);
}

GCC output is:

ui = 3318069411 (0xc5c5b8a3)            UL-ui = -5787213829993660416 (0xafafafaf00000000)

LLVM output is:

ui = 3318069411 (0xc5c5b8a3)            UL-ui = 0 (0xafafafaf)

The problem is the way to pass arguments to printf.
GCC asm:

	li	a4,-1347440640
	addi	sp,sp,-24
	addi	a4,a4,-81
	sw	a4,8(sp)
	lw	a5,8(sp)
	li	a2,-976896000
	addi	a2,a2,-1885
	lui	a0,%hi(.LC1)
	sw	a5,0(sp)
	li	a3,0
	li	a5,0
	mv	a1,a2
	addi	a0,a0,%lo(.LC1)
	sw	ra,20(sp)
	sw	a3,4(sp)
	call	printf

LLVM asm:

	addi	sp, sp, -16
	sw	ra, 12(sp)                      # 4-byte Folded Spill
	sw	s0, 8(sp)                       # 4-byte Folded Spill
	addi	s0, sp, 16
	andi	sp, sp, -8
	lui	a0, 719611
	addi	a5, a0, -81
	sw	a5, 4(sp)
	lui	a0, %hi(.L.str)
	addi	a0, a0, %lo(.L.str)
	lui	a1, 810076
	addi	a1, a1, -1885
	sw	zero, 0(sp)
	mv	a2, a1
	mv	a4, zero
	call	printf

Both GCC and LLVM pass format string and two ui by a0, a1, a2, the difference is how they pass rest variadic arguments.
UL-ui is with 2*XLEN size, so it will be spilt to two part (low and high 32-bits). Low part is 0x00000000, high part is 0xafafafaf.
For GCC:

First UL-ui: low -> a3, high -> a4
Second UL-ui: low -> a5, high -> stack.0

For LLVM:

First UL-ui: low -> a4, high -> a5
Second UL-ui: low -> stack.0, high -> stack.1

Because we use GLIBC compiled by GCC while linking with LLVM's output, so in printf's view:

a3 -> undefined, so it is zero.
a4 -> low part, 0x00000000
a5 -> high part, 0xafafafaf
stack.0 -> low part, 0x00000000
stack.1 -> not used

It get 0x0000000000000000 and 0x00000000afafafaf for two UL-ui (seen as the output).

In the ABI specification, it says (Integer Calling Convention):

In the base integer calling convention, variadic arguments are passed in the same manner as named arguments, with one exception. Variadic arguments with 2ร—XLEN-bit alignment and size at most 2ร—XLEN bits are passed in an aligned register pair (i.e., the first register in the pair is even-numbered), or on the stack by value if none is available. After a variadic argument has been passed on the stack, all future arguments will also be passed on the stack (i.e. the last argument register may be left unused due to the aligned register pair rule).

And this is what LLVM do for ILP32E currently.

I saw the same issue on Github(Inconsistent variadic argument passing behavior between ilp32 and ilp32e for long long/double), so shall LLVM be compatible with GCC's behavior?
@kito-cheng @khchen

โ€ข pcwang-thead added a subscriber: lenary.
  • Support FastCC and disable GHC on RV32E.
  • Don't use aligned registers to pass variadic arguments.
  • Set stack alignment to 4-bytes for types with length of 2*XLEN.
  • Add some tests.
โ€ข pcwang-thead edited the summary of this revision. (Show Details)Mar 25 2022, 6:15 AM
โ€ข pcwang-thead edited the summary of this revision. (Show Details)
โ€ข pcwang-thead removed a subscriber: lenary.

Rebase and fix test errors.

zixuan-wu added inline comments.Oct 10 2022, 2:21 AM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
116

I am wondering whether we need construct another new RegisterClass for RV32E instead of GPR, for example eGPR, so that the num and other info such as weight, etc of RegisterClass can adjust. Then the reserved logic is not necessary.

zixuan-wu added inline comments.Oct 17 2022, 4:14 AM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
515โ€“519

I think this 16 should be adjusted as above logic for rv32e

  • Rebase.
  • Align libcall stack size to getStackAlign().
โ€ข pcwang-thead marked an inline comment as done.Oct 17 2022, 5:42 AM
โ€ข pcwang-thead added inline comments.
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
515โ€“519

Thanks. :-)

luojia added a subscriber: luojia.Oct 20 2022, 8:57 PM

Hello! Any further updates to this patch? It seems like all the inline comments have been resolved.

โ€ข pcwang-thead marked an inline comment as done.Oct 21 2022, 2:25 AM

Hello! Any further updates to this patch? It seems like all the inline comments have been resolved.

We have done some works in this patch to make it compatible with GCC, it can be combined with GNU toolchain now.

But as what have been discussed[1, 2], we may proceed with this patch when RV32E/ilp32e is ratified.

  1. https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/269
  2. https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/257

Hello! Any further updates to this patch? It seems like all the inline comments have been resolved.

We have done some works in this patch to make it compatible with GCC, it can be combined with GNU toolchain now.

But as what have been discussed[1, 2], we may proceed with this patch when RV32E/ilp32e is ratified.

  1. https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/269
  2. https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/257

RV32E/ilp32e has been ratified(https://github.com/riscv-non-isa/riscv-elf-psabi-doc). Do you plan to proceed with this patch? :)

Hello! Any further updates to this patch? It seems like all the inline comments have been resolved.

We have done some works in this patch to make it compatible with GCC, it can be combined with GNU toolchain now.

But as what have been discussed[1, 2], we may proceed with this patch when RV32E/ilp32e is ratified.

  1. https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/269
  2. https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/257

RV32E/ilp32e has been ratified(https://github.com/riscv-non-isa/riscv-elf-psabi-doc). Do you plan to proceed with this patch? :)

I will follow the proceeding of spec and finish this patch, but I don't think they have been ratified.
There are some changes about RV32E/RV64E, but I think they are still proposal.
And, there are still some issues we need to fix in the psabi:

Rebase.

If D143570 is committed, I will do some works to support of RV64E/lp64e.

74th added a subscriber: 74th.Feb 27 2023, 4:41 AM
hlvlad added a subscriber: hlvlad.Mar 4 2023, 3:04 AM

Hi, I'm working on CH32V003 for rust and it uses RV32EC core.
I tried replacing my distros llvm and clang with a patched version of this like this:

git clone https://aur.archlinux.org/llvm-git.git
cd llvm-git
mkdir src
cd src
git clone https://github.com/llvm/llvm-project.git
cd llvm-project
arc patch D70401
cd ../..
mv llvm-config.h src/
makepkg -es
sudo pacman -Rd --nodeps clang llvm
makepkg -eid

but that bricked my xfce-wayland-manjaro DE (one screen black)
And in config.toml if I put

[build]
target = "riscv32i-unknown-none-elf"
rustflags = [
	"-C", "target-feature=+e,+c"
]

then build with cargo build
LLVM still complains it doesn't implement CodeGen for RV32E yet
What am I doing wrong?
Ended up reverting to repository llvm and clang, desktop now works again but CodeGen is obviously missing.

Hi, I'm working on CH32V003 for rust and it uses RV32EC core.
I tried replacing my distros llvm and clang with a patched version of this like this:

git clone https://aur.archlinux.org/llvm-git.git
cd llvm-git
mkdir src
cd src
git clone https://github.com/llvm/llvm-project.git
cd llvm-project
arc patch D70401
cd ../..
mv llvm-config.h src/
makepkg -es
sudo pacman -Rd --nodeps clang llvm
makepkg -eid

but that bricked my xfce-wayland-manjaro DE (one screen black)
And in config.toml if I put

[build]
target = "riscv32i-unknown-none-elf"
rustflags = [
	"-C", "target-feature=+e,+c"
]

then build with cargo build
LLVM still complains it doesn't implement CodeGen for RV32E yet
What am I doing wrong?
Ended up reverting to repository llvm and clang, desktop now works again but CodeGen is obviously missing.

I don't see any obvious problem here.
I am not familiar with rust. Is riscv32i-unknown-none-elf a valid target for rustc, it should be something like riscv32-unknown-elf in LLVM I think. And is target-feature=+e,+c the right way to specify features?
Can you please provide the whole command/arguments passed to LLVM?

  • Rebase.
  • Add base support of RV64E/lp64e.

Any feedbacks are welcomed.

โ€ข pcwang-thead retitled this revision from [RISCV] Complete RV32E/ilp32e implementation to [RISCV] CodeGen of RVE and ilp32e/lp64e ABIs.Mar 27 2023, 4:47 AM
โ€ข pcwang-thead edited the summary of this revision. (Show Details)

Hi, I'm working on CH32V003 for rust and it uses RV32EC core.
I tried replacing my distros llvm and clang with a patched version of this like this:

git clone https://aur.archlinux.org/llvm-git.git
cd llvm-git
mkdir src
cd src
git clone https://github.com/llvm/llvm-project.git
cd llvm-project
arc patch D70401
cd ../..
mv llvm-config.h src/
makepkg -es
sudo pacman -Rd --nodeps clang llvm
makepkg -eid

but that bricked my xfce-wayland-manjaro DE (one screen black)
And in config.toml if I put

[build]
target = "riscv32i-unknown-none-elf"
rustflags = [
	"-C", "target-feature=+e,+c"
]

then build with cargo build
LLVM still complains it doesn't implement CodeGen for RV32E yet
What am I doing wrong?
Ended up reverting to repository llvm and clang, desktop now works again but CodeGen is obviously missing.

I don't see any obvious problem here.
I am not familiar with rust. Is riscv32i-unknown-none-elf a valid target for rustc, it should be something like riscv32-unknown-elf in LLVM I think. And is target-feature=+e,+c the right way to specify features?
Can you please provide the whole command/arguments passed to LLVM?

Yeah so I looked at the at the target files of rustc, telling rustc to do RV32I will indeed result in RV32 and the way to enable the E and C features seems to be correct, BUT:
rust uses their own "special sauce" version of llvm and rustc needs to be built against that to enable the new features. I tried to apply (patch) the diff directly to rusts llvm branch but there were many errors, and I couldn't figure out how to apply them manually since some things are different.
I'm stuck, this is all way beyond my understanding. Sorry I can't test it for you guys.
What I did was:

git clone https://github.com/rust-lang/rust.git
cd rust
nvim config.toml
[llvm]
download-ci-llvm = false

then I started building with

./x.py build

and as soon as the rust-llvm source was downloaded completely I aborted (CTRL+C).

then downloaded the raw diff from this page (button top right) into the rust llvm dir, opened a terminal in that dir and tried to patch with

patch -p1 < D70401.diff

but that gives lots of errors
resolving them manually seems way beyond me, especially since patch seems to already use fuzzy matching

Hi, I'm working on CH32V003 for rust and it uses RV32EC core.
I tried replacing my distros llvm and clang with a patched version of this like this:

git clone https://aur.archlinux.org/llvm-git.git
cd llvm-git
mkdir src
cd src
git clone https://github.com/llvm/llvm-project.git
cd llvm-project
arc patch D70401
cd ../..
mv llvm-config.h src/
makepkg -es
sudo pacman -Rd --nodeps clang llvm
makepkg -eid

but that bricked my xfce-wayland-manjaro DE (one screen black)
And in config.toml if I put

[build]
target = "riscv32i-unknown-none-elf"
rustflags = [
	"-C", "target-feature=+e,+c"
]

then build with cargo build
LLVM still complains it doesn't implement CodeGen for RV32E yet
What am I doing wrong?
Ended up reverting to repository llvm and clang, desktop now works again but CodeGen is obviously missing.

I don't see any obvious problem here.
I am not familiar with rust. Is riscv32i-unknown-none-elf a valid target for rustc, it should be something like riscv32-unknown-elf in LLVM I think. And is target-feature=+e,+c the right way to specify features?
Can you please provide the whole command/arguments passed to LLVM?

Yeah so I looked at the at the target files of rustc, telling rustc to do RV32I will indeed result in RV32 and the way to enable the E and C features seems to be correct, BUT:
rust uses their own "special sauce" version of llvm and rustc needs to be built against that to enable the new features. I tried to apply (patch) the diff directly to rusts llvm branch but there were many errors, and I couldn't figure out how to apply them manually since some things are different.
I'm stuck, this is all way beyond my understanding. Sorry I can't test it for you guys.
What I did was:

git clone https://github.com/rust-lang/rust.git
cd rust
nvim config.toml
[llvm]
download-ci-llvm = false

then I started building with

./x.py build

and as soon as the rust-llvm source was downloaded completely I aborted (CTRL+C).

then downloaded the raw diff from this page (button top right) into the rust llvm dir, opened a terminal in that dir and tried to patch with

patch -p1 < D70401.diff

but that gives lots of errors
resolving them manually seems way beyond me, especially since patch seems to already use fuzzy matching

So it seems that rust uses its own llvm branch based on released llvm branch, so I think you may download old version of this patch which is near the baseline of rust llvm branch and try again. :-)

This comment was removed by kekcheburec.

Hey I've tried using this patch (roughly following https://noxim.xyz/blog/rust-ch32v003/).

It uses the older version of this patch for the rust llvm version (here the llvm tree https://github.com/Noxime/llvm-project/tree/rv32e) and I use rust commit 0939ec13 (together with the small patch for the RVE).

I've experience some issues that results in corruption of $sp, the following is the smallest reproduction (hopefully small enough):
Code:

rust
#![no_std]

pub fn test()  {
}

which, with the following .ll for release builds:

source_filename = "miscomp_repro.8b6a426d3b54bd13-cgu.0"
target datalayout = "e-m:e-p:32:32-i64:64-n32-S128"
target triple = "riscv32"

define dso_local void @_ZN13miscomp_repro4test17h065760f827b95d43E() unnamed_addr #0 {
start:
  ret void
}

attributes #0 = { mustprogress nofree norecurse nosync nounwind readnone willreturn "target-cpu"="generic-rv32" "target-features"="+e,+c" }

results in this assembly:

	.text
	.attribute	4, 4
	.attribute	5, "rv32e1p9_c2p0"
	.file	"miscomp_repro.8b6a426d3b54bd13-cgu.0"
	.section	.text._ZN13miscomp_repro4test17h065760f827b95d43E,"ax",@progbits
	.globl	_ZN13miscomp_repro4test17h065760f827b95d43E
	.p2align	1
	.type	_ZN13miscomp_repro4test17h065760f827b95d43E,@function
_ZN13miscomp_repro4test17h065760f827b95d43E:
	mv	sp, s0
	ret
.Lfunc_end0:
	.size	_ZN13miscomp_repro4test17h065760f827b95d43E, .Lfunc_end0-_ZN13miscomp_repro4test17h065760f827b95d43E

	.section	".note.GNU-stack","",@progbits

Since s0 isn't required to have any specific contents (and in the larger project this was extracted from doesn't), this corrupts the stack pointer. Large functions using the stack first save sp to 0, so not all functions have this issue. This also happens (but more verbose) in debug builds, but works fine with the exact same toolchain using the riscv32i target.

Here is the repro with some further output, I hope this patch and not something else is to blame (if so, sorry in advance).

Hey I've tried using this patch (roughly following https://noxim.xyz/blog/rust-ch32v003/).

It uses the older version of this patch for the rust llvm version (here the llvm tree https://github.com/Noxime/llvm-project/tree/rv32e) and I use rust commit 0939ec13 (together with the small patch for the RVE).

I've experience some issues that results in corruption of $sp, the following is the smallest reproduction (hopefully small enough):
Code:

rust
#![no_std]

pub fn test()  {
}

which, with the following .ll for release builds:

source_filename = "miscomp_repro.8b6a426d3b54bd13-cgu.0"
target datalayout = "e-m:e-p:32:32-i64:64-n32-S128"
target triple = "riscv32"

define dso_local void @_ZN13miscomp_repro4test17h065760f827b95d43E() unnamed_addr #0 {
start:
  ret void
}

attributes #0 = { mustprogress nofree norecurse nosync nounwind readnone willreturn "target-cpu"="generic-rv32" "target-features"="+e,+c" }

results in this assembly:

	.text
	.attribute	4, 4
	.attribute	5, "rv32e1p9_c2p0"
	.file	"miscomp_repro.8b6a426d3b54bd13-cgu.0"
	.section	.text._ZN13miscomp_repro4test17h065760f827b95d43E,"ax",@progbits
	.globl	_ZN13miscomp_repro4test17h065760f827b95d43E
	.p2align	1
	.type	_ZN13miscomp_repro4test17h065760f827b95d43E,@function
_ZN13miscomp_repro4test17h065760f827b95d43E:
	mv	sp, s0
	ret
.Lfunc_end0:
	.size	_ZN13miscomp_repro4test17h065760f827b95d43E, .Lfunc_end0-_ZN13miscomp_repro4test17h065760f827b95d43E

	.section	".note.GNU-stack","",@progbits

Since s0 isn't required to have any specific contents (and in the larger project this was extracted from doesn't), this corrupts the stack pointer. Large functions using the stack first save sp to 0, so not all functions have this issue. This also happens (but more verbose) in debug builds, but works fine with the exact same toolchain using the riscv32i target.

Here is the repro with some further output, I hope this patch and not something else is to blame (if so, sorry in advance).

Thanks for reporting this.
I tried to compile your .ll on my local machine with newest patch, I didn't see the problem. I don't know if it is the bug in older version of this patch, so I suggest you to update the patch and try again. :-)
By the way, you can provide the log when you compile the .ll with -mllvm -print-after-all option (and -mllvm -debug if your llvm is a debug build). It can be helpful for me to figure out which part is wrong.

Thank you for the reply, I've compiled this with the most recent patch and also didn't see a problem (but can't get it running with rustc). Building the .ll with the older patch, the same issue also occurs, so I *do* think its the old patch version?
Here is the log output for the riscv32e

(and as a sanity check riscv32i)

The errant code seems to get introduced here:

# *** IR Dump After Prologue/Epilogue Insertion & Frame Finalization (prologepilog) ***:
# Machine code for function _ZN13miscomp_repro4test17h065760f827b95d43E: NoPHIs, TracksLiveness, NoVRegs, TiedOpsRewritten, TracksDebugUserValues

bb.0.start:
  $x2 = frame-destroy ADDI $x8, 0
  PseudoRET

Thank you for the reply, I've compiled this with the most recent patch and also didn't see a problem (but can't get it running with rustc). Building the .ll with the older patch, the same issue also occurs, so I *do* think its the old patch version?
Here is the log output for the riscv32e

(and as a sanity check riscv32i)

The errant code seems to get introduced here:

# *** IR Dump After Prologue/Epilogue Insertion & Frame Finalization (prologepilog) ***:
# Machine code for function _ZN13miscomp_repro4test17h065760f827b95d43E: NoPHIs, TracksLiveness, NoVRegs, TiedOpsRewritten, TracksDebugUserValues

bb.0.start:
  $x2 = frame-destroy ADDI $x8, 0
  PseudoRET

Thanks! It seems that the problem is that we do wrong FP adjustment here.
But, as you can see, this patch does almost nothing to RISCVFrameLowering. So I think the bug may have been fixed somewhere else (I do remember there is a bug fix but I can't remember the differential ID).
So I would suggest you to use newest patch or do some bitsecting to find the bug fix commit if you don't bother. :-)

evandro removed a subscriber: evandro.Jun 12 2023, 2:33 PM
zixuan-wu added inline comments.Jul 7 2023, 2:10 AM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
998

Hi, @wangpc it's hidden bug that out of range registers are saved/restored in prologue/epilogue

wangpc added inline comments.Jul 7 2023, 3:19 AM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
998

Thanks! We don't need to save X16-X31 for interrupt functions.

zixuan-wu added inline comments.Jul 7 2023, 3:21 AM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
66โ€“69

Here also need adjust for rve.

wangpc commandeered this revision.Jul 8 2023, 11:55 AM
wangpc added a reviewer: โ€ข pcwang-thead.
wangpc removed reviewers: lenary, โ€ข pcwang-thead.
wangpc updated this revision to Diff 538547.Jul 10 2023, 2:13 AM
  • Rebase.
  • Fix PEI bugs of interrupt functions.
koute added a subscriber: koute.Aug 17 2023, 7:35 AM
koute added a comment.Sep 1 2023, 9:07 PM

I know that there are still open issues regarding the psABI, but considering how slow it's been going, couldn't we merge this in anyway and mark it as experimental and subject to change? Please?

The patch is simple enough to not become a maintenance burden, and GCC already has it even though the ABI's unfinished, and the RV32E target itself is most likely going to be used for standalone bare metal programs where the exact ABI shouldn't matter too much as long as it works.

I'm asking because I'd really like to have this merged so that I could use Rust to target RV32E/RV64E. Right now I have to maintain my own toolchain, which is painful; if this got merged (even in an experimental fashion, like GCC has) I could just get upstream Rust to support it out-of-box.

wangpc added a comment.Sep 2 2023, 7:52 AM

I know that there are still open issues regarding the psABI, but considering how slow it's been going, couldn't we merge this in anyway and mark it as experimental and subject to change? Please?

The patch is simple enough to not become a maintenance burden, and GCC already has it even though the ABI's unfinished, and the RV32E target itself is most likely going to be used for standalone bare metal programs where the exact ABI shouldn't matter too much as long as it works.

I'm asking because I'd really like to have this merged so that I could use Rust to target RV32E/RV64E. Right now I have to maintain my own toolchain, which is painful; if this got merged (even in an experimental fashion, like GCC has) I could just get upstream Rust to support it out-of-box.

@asb @kito-cheng @jrtc27 What do you think about?

craig.topper added inline comments.Sep 2 2023, 9:29 PM
clang/lib/CodeGen/Targets/RISCV.cpp
491

4-bytes -> 4-byte

clang/test/Preprocessor/riscv-target-features.c
6

__riscv_64e too

llvm/lib/Support/RISCVISAInfo.cpp
987โ€“989

This needs to be rebased. These FIXMEs were removed.

I know that there are still open issues regarding the psABI, but considering how slow it's been going, couldn't we merge this in anyway and mark it as experimental and subject to change? Please?

The patch is simple enough to not become a maintenance burden, and GCC already has it even though the ABI's unfinished, and the RV32E target itself is most likely going to be used for standalone bare metal programs where the exact ABI shouldn't matter too much as long as it works.

I'm asking because I'd really like to have this merged so that I could use Rust to target RV32E/RV64E. Right now I have to maintain my own toolchain, which is painful; if this got merged (even in an experimental fashion, like GCC has) I could just get upstream Rust to support it out-of-box.

I agree. Lots of our Rust work on low-level RISC-V cores (embedded, monitor hart, etc.) rely on RVE and they depend on RVE support on LLVM. We've waited for LLVM upstream support for an amount of years; considering how much time the community have waited for, RVE codegen can be accepted even if it's marked experimental.

wangpc updated this revision to Diff 556927.Sep 17 2023, 11:32 PM
  • Rebase.
  • Address comments.
  • Add ReleaseNotes.
wangpc marked 8 inline comments as done.Sep 17 2023, 11:39 PM

ping?

Pong ๐Ÿ˜‚

zixuan-wu accepted this revision.Oct 27 2023, 1:27 AM

LGTM if nobody objects.

This revision is now accepted and ready to land.Oct 27 2023, 1:27 AM

@asb @kito-cheng @jrtc27 @craig.topper
Can I commit this since the support of RVE is really of great importance for some downstreams? If there are some problems, I will be there to fix them.
If we all agree with this, I will mark RVE as exprimental and commit it then.

craig.topper added inline comments.Oct 27 2023, 8:25 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
17586

This code has been rewritten recently. Please rebase

asb added a comment.Oct 27 2023, 8:55 AM

First of all, thank you to everyone who has been trying to nudge this forward and apologies it must have been a frustrating experience.

I appreciate there are users who want to see this and I don't like that LLVM doesn't serve them right now - I think it's unfortunate that this need for the ABI hasn't translated into effort to finalise the ABI definition in the psABI doc and to at least get it to match what GCC actually implements (spec. That said, I've not really vocalised that concern clearly up to now - so that's my bad.

Matching what GCC does by setting stack alignment to 4 bytes for 2xlen types seems fine - except this doesn't seem to be documented explicitly in the current ABI doc (it notes the stack if 4 byte aligned, but you could have that be the case and still require it to be realigned when storing objects with a greater alignment requirement, surely?).

Having different alignment requirements _only_ on the stack does seem ugly, but I can't think of something off hand that would realistically break with this.

@wangpc do you want to update this with the suggested documentation in the release notes and RISCVUsage on the support being "experimental"?

wangpc updated this revision to Diff 557932.Oct 30 2023, 12:41 AM
  • Rebase.
  • Mask as experimental.
In D70401#4655408, @asb wrote:

First of all, thank you to everyone who has been trying to nudge this forward and apologies it must have been a frustrating experience.

I appreciate there are users who want to see this and I don't like that LLVM doesn't serve them right now - I think it's unfortunate that this need for the ABI hasn't translated into effort to finalise the ABI definition in the psABI doc and to at least get it to match what GCC actually implements (spec. That said, I've not really vocalised that concern clearly up to now - so that's my bad.

Matching what GCC does by setting stack alignment to 4 bytes for 2xlen types seems fine - except this doesn't seem to be documented explicitly in the current ABI doc (it notes the stack if 4 byte aligned, but you could have that be the case and still require it to be realigned when storing objects with a greater alignment requirement, surely?).

Having different alignment requirements _only_ on the stack does seem ugly, but I can't think of something off hand that would realistically break with this.

@wangpc do you want to update this with the suggested documentation in the release notes and RISCVUsage on the support being "experimental"?

Thanks! I added a note to the RISCVUsage. There won't be experimental-e like other experimental extensions as it is already ratified and adds no instruction, it is experimental just because the support is experimental.

For ABI part, I don't know if @kito-cheng has some updates/comments.

asb added a comment.Oct 30 2023, 2:49 AM

Thanks, I'll take another look. Rereading my previous comment I just wanted to clarify one part so it's not misunderstood. I said " I think it's unfortunate that this need for the ABI hasn't translated into effort to finalise the ABI definition in the psABI doc and to at least get it to match what GCC actually implements" - I wanted to be very clear this isn't a criticism of those trying to maintain the ABI doc, it's about companies who want to ship RVE hardware and software not contributing to that process.

koute added a comment.Nov 18 2023, 3:17 AM

Sorry for the comment spam, but could we please get this merged in finally? (:

To people who hold the decision making power as to whether this is merged: are there still any blockers left, considering the consensus was to merge it? What's the hold up? Is there anything I can do to help?

@craig.topper Thanks!
@asb Hi Alex, I'd like to get another approval from you. Are there any more concerns?

GCC only ever defines __riscv_32e

clang/lib/Basic/Targets/RISCV.cpp
215

Ugh, these don't align with the normal pattern. riscv_e already exists in the spec, can we just leave riscv_32e as deprecated for RV32E and not introduce the old-style __riscv_64e?

clang/lib/Basic/Targets/RISCV.h
142

Does it matter we don't undo the effects of the RVE ABI here?

GCC only ever defines __riscv_32e

Hm, seems the comments about __riscv_32e were from months ago, ignore them if they aren't correct or have become outdated...

GCC only ever defines __riscv_32e

Hm, seems the comments about __riscv_32e were from months ago, ignore them if they aren't correct or have become outdated...

FYI: https://github.com/riscv-non-isa/riscv-c-api-doc/pull/52

Hello, could we please merge this ๐Ÿ™‚?

koute added a comment.Dec 18 2023, 5:37 AM

Again, sorry for the spam, but I second @hlvlad; it's been a month since I last commented and we still made no progress in merging this PR. Could we please somehow get this merged before the next LLVM branch point in January? I really don't want this to be delayed yet another half a year, and time's running out. At this point I don't care if I'm going to annoy people; I'm willing to do whatever it takes to help and drive this forward.

@asb: Could we please get an extra review and/or approval as requested here?

@asb Hi Alex, I'd like to get another approval from you. Are there any more concerns?