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
jrtc27 added inline comments.Jan 14 2021, 4:08 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9372

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

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

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
9575

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
11 ↗(On Diff #229858)

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
106

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
26

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)
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.