llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.7k stars 11.87k forks source link

RISC-V: ILP32E cannot be used with the D ISA extension #100814

Open patrick-rivos opened 3 months ago

patrick-rivos commented 3 months ago

LLVM differs from GCC in its support of the D extension when using -mabi=ilp32e. Godbolt: https://godbolt.org/z/En7r85Gex Assert: https://github.com/llvm/llvm-project/blob/069e8bcd82c4420239f95c7e6a09e1f756317cfc/clang/lib/Basic/Targets/RISCV.cpp#L364

From the original merge request:

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.

https://reviews.llvm.org/D70401

If I'm reading this comment correctly, D can be allowed with -mabi=ilp32e (GCC currently allows it). Is the current behavior intentional?

cc @kito-cheng

llvmbot commented 3 months ago

@llvm/issue-subscribers-backend-risc-v

Author: Patrick O'Neill (patrick-rivos)

LLVM differs from GCC in its support of the D extension when using -mabi=ilp32e. Godbolt: https://godbolt.org/z/En7r85Gex Assert: https://github.com/llvm/llvm-project/blob/069e8bcd82c4420239f95c7e6a09e1f756317cfc/clang/lib/Basic/Targets/RISCV.cpp#L364 From the original merge request: >> 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. https://reviews.llvm.org/D70401 If I'm reading this comment correctly, D can be allowed with -mabi=ilp32e (GCC currently allows it). Is the current behavior intentional? cc @kito-cheng
topperc commented 3 months ago

I played around with gcc briefly. I'm not convinced the fld in this test case isn't possibly misaligned https://godbolt.org/z/fMxdrxhM1. ILP32E only guarantees 4 bytes stack alignment and I don't see the usual ANDI for aligning the stack pointer.

lenary commented 3 months ago

Broadly, GCC's codegen here is wrong (for the alignment reasons that craig pointed out), and the ABI has called this out specifically as an incompatible combination:

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.

(From https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc#ilp32e-calling-convention)

I believe this restriction was added to the ABI around the time when the patch you linked above was merged, because it cannot easily be made to work (you end up requiring more callee-saved registers to hold pointers to various parts of your stack frame than you have).

I think the point being made by the comment you point out is that ilp32e is compatible with cores that implement D, so long as you don't write any code at all that needs to use or spill or call via a D register - and the easiest way to manage that is to compile as if the D extension is not present.

patrick-rivos commented 3 months ago

Thanks @topperc and @lenary! Sounds like LLVM's behavior is intentional. I'll start a conversation on the GCC side to see if we want to similarly disallow 'D' on ilp32e.

Edit: GCC bugzilla: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116111

kito-cheng commented 3 months ago

In short: Yeah, I think it's a GCC bug, but I am not sure what's the right direction to fix that either.

Longer version: I feel a responsibility to tell this story since I am one of the main authors of the GCC implementation and the author of the psABI ilp32e draft*1. With many new friends joining the RISC-V development, it's time to talk about some history here.

First, RV32E was not supported for use with F and D[1]. Therefore, the design of ilp32e did not consider the alignment of the fld instruction at all. The only goal of RV32E was to minimize the memory footprint, so the stack alignment was set to 4 bytes.

The constraints for the E extension have been relaxed, so we should update the ilp32e draft. A few ideas have been proposed in the past years: 1) increasing the stack alignment to 8 bytes or 16 bytes, or 2) adding new ABI variants like ilp32ef and ilp32ed, while also restricting ilp32e to be used only without D.

BUT...not enough people are interested in the ilp32e ABI, so it's been stuck for a while.

Lastly, here is a call for help. Let me know if anyone is interested in the ilp32e and/or lp64e ABI. Since the E extension has become ratified, we should move this forward.

*1 Yes, ilp32e is still under draft status. One major reason is that rv32e was under version 1.9 for a long time, which meant it could still change. rv32e became version 2.0 last year[3].

[1] Relax the constraint of the E extension, allowing rv64e and combinations with F and D extensions. https://github.com/riscv/riscv-isa-manual/commit/4845f5d61f96a827ec4d21d2c5a80b6bf7881e56 [2] Bump version from 1.9 to 1.95 https://github.com/riscv/riscv-isa-manual/commit/ec4919caf5638e2d1c437391c6eab097e038aaec [3] Bump version from 1.95 to 2.0 https://github.com/riscv/riscv-isa-manual/commit/fefbe61f8ffa04d6942fc5493ef65f3f66e48170