Page MenuHomePhabricator

[RISCV] Complete RV32E/ilp32e implementation
Needs ReviewPublic

Authored by pcwang-thead on Nov 18 2019, 8:34 AM.

Details

Summary

This commit includes the necessary changes to clang and LLVM to support
codegen of RV32E and the ilp32e ABI.
The differences between RV32E and RV32I are:

  • RV32E reduces the integer register count to 16(x0-x15).
  • The ABI should be ilp32e.

RV32E can be combined with all current standard extensions.
The central changes in ilp32e ABI, compared to ilp32 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).
  • Not compatible with D ISA extension.

If ilp32e 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 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 RV32E, 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

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)
pcwang-thead added reviewers: craig.topper, khchen.
  • 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)
  • Update tests.
  • Fix tests.
pcwang-thead retitled this revision from [WIP][RISCV] Complete RV32E/ilp32e implementation to [RISCV] Complete RV32E/ilp32e implementation.Dec 27 2021, 10:10 PM

Update tests.

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?

pcwang-thead added inline comments.Jan 17 2022, 10:04 PM
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
112

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
461–462

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
461–462

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.Mon, Feb 27, 4:41 AM
hlvlad added a subscriber: hlvlad.Sat, Mar 4, 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?