Page MenuHomePhabricator

RISC-V big-endian support implementation
Needs ReviewPublic

Authored by gbenyei on Jun 26 2022, 8:20 AM.

Details

Summary

Implement riscv32be and riscv64be targets.
The RISC-V big- and bi-endian targets are discussed in the RISC-V spec Version 20191213, but some aspects, like ABI are still unclear.
The instruction encoding is little endian in both big- and little-endian modes. ISA spec Volume 1 1.15: "Instructions are stored in memory as a sequence
of 16-bit little-endian parcels, regardless of memory system endianness".

RISC-V Big-endian cores are already supported by GCC. Where spec is unclear, we aim to be compatible with GCC.

Diff Detail

Event Timeline

gbenyei created this revision.Jun 26 2022, 8:20 AM
gbenyei requested review of this revision.Jun 26 2022, 8:20 AM
craig.topper added inline comments.
clang/lib/Basic/Targets/RISCV.h
111

Instead of creating new classes, could we have a branch on the Arch or isLittleEndian portion of the triple to decide what to pass to resetDataLayout? That's what PPC32TargetInfo does for example.

lld/ELF/Arch/RISCV.cpp
160 ↗(On Diff #440067)

Is it possible to use write64 instead of write64le/be? It looks like it checks the endianness internally.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
628

Capitalize swapValue

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
32–33

Is this longer than 80 columns?

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCAsmInfo.cpp
24

Could we do IsLittleEndian = TT.isLittleEndian()?

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
64

No else after return

jhenderson added inline comments.Jun 27 2022, 1:14 AM
llvm/include/llvm/ADT/Triple.h
864

Perhaps worth updating to mention big and little endian here, like isPPC64 above?

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
304–305

We need llvm-objcopy testing for these new targets.

gbenyei updated this revision to Diff 440196.Jun 27 2022, 6:25 AM

Thanks, Craig. Updated the patch with your remarks.

jrtc27 added inline comments.Jun 27 2022, 6:41 AM
clang/lib/Basic/Targets/RISCV.h
113

And please avoid repeating the whole data layout, just make the e/E a variable

clang/lib/Driver/ToolChains/FreeBSD.cpp
235

-X to match LE. Or ditch these (and I can ditch riscv32...) since FreeBSD only supports riscv64.

clang/lib/Driver/ToolChains/Gnu.cpp
1701–1702

Just shove a + Be + in the middle of these two rather than introducing a whole new set and picking between them?

llvm/cmake/config-ix.cmake
463 ↗(On Diff #440196)

I believe these need to come before the unsuffixed versions to do anything, but also the unsuffixed versions already handle the suffixed versions correctly so this isn't needed?

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp
554 ↗(On Diff #440196)

Why switch to this order when before you've used 32, 64, 32be, 64be as the order

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
65

As with Clang

gbenyei marked 6 inline comments as done.Jun 27 2022, 7:05 AM
gbenyei updated this revision to Diff 440579.Jun 28 2022, 5:43 AM
gbenyei marked 7 inline comments as done.

Objcopy aspects look good, thanks.

lld/ELF change should be dropped from this change. Don't use config->endianness.
I feel sad that for little-endian users who don't use big-endian, every write now is slightly slower due to a check ;-)

clang/lib/Basic/Targets/RISCV.cpp
123–124

The convention doesn't add () in such an assignment.

218

This can be simplified with something like isRISCV64()

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

You may use a char and possibly fold this into the expression below.

143

delete blank line

Objcopy aspects look good, thanks.

Thanks

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp
554 ↗(On Diff #440196)

The order in the code before my changes is 64, 32. I guess for no good reason, but I prefer not to re-order code while implementing a feature - it trashes git history.

lld/ELF change should be dropped from this change. Don't use config->endianness.
I feel sad that for little-endian users who don't use big-endian, every write now is slightly slower due to a check ;-)

Hi, I'm not sure I get it. How will we have a fully functional toolchain, if I don't implement the lld/ELF part?
In LLVM, unlike in GCC, target related decisions happen in runtime. I think it's a high level design decision. While I can understand the pain of LE developers getting a slightly slower linker due to endianness checking, I sure will feel the pain of a BE developer not having a linker...

Please explain why I shouldn't use config->endianness?

MaskRay added a comment.EditedJun 29 2022, 6:18 PM

lld/ELF change should be dropped from this change. Don't use config->endianness.
I feel sad that for little-endian users who don't use big-endian, every write now is slightly slower due to a check ;-)

Hi, I'm not sure I get it. How will we have a fully functional toolchain, if I don't implement the lld/ELF part?
In LLVM, unlike in GCC, target related decisions happen in runtime. I think it's a high level design decision. While I can understand the pain of LE developers getting a slightly slower linker due to endianness checking, I sure will feel the pain of a BE developer not having a linker...

Please explain why I shouldn't use config->endianness?

See PPC64.cpp. See D96188 how I added aarch64_be support. A set of representative tests should be picked with be tests.
If llvm-project consensus is that we will add big-endian support, I can handle lld/ELF part. I am mostly concerned with this scenarios that some RISC-V folks click LGTM, and the change lands with no test in some areas, or the code somewhat breaks local convention.

Many of the changes in this patch probably should be split. llvm-objcopy and JIT changes definitely needs appropriate tests and the suitable domain reviewers.

gbenyei marked 3 inline comments as done.Jun 29 2022, 9:38 PM

lld/ELF change should be dropped from this change. Don't use config->endianness.
I feel sad that for little-endian users who don't use big-endian, every write now is slightly slower due to a check ;-)

Hi, I'm not sure I get it. How will we have a fully functional toolchain, if I don't implement the lld/ELF part?
In LLVM, unlike in GCC, target related decisions happen in runtime. I think it's a high level design decision. While I can understand the pain of LE developers getting a slightly slower linker due to endianness checking, I sure will feel the pain of a BE developer not having a linker...

Please explain why I shouldn't use config->endianness?

See PPC64.cpp. See D96188 how I added aarch64_be support. A set of representative tests should be picked with be tests.
If llvm-project consensus is that we will add big-endian support, I can handle lld/ELF part. I am mostly concerned with this scenarios that some RISC-V folks click LGTM, and the change lands with no test in some areas, or the code somewhat breaks local convention.

Many of the changes in this patch probably should be split. llvm-objcopy and JIT changes definitely needs appropriate tests and the suitable domain reviewers.

Thanks, it makes more sense now. I'll split the LLD changes, and remove the JIT related stuff.

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

Concatenating a conditional char and a string literal might be tricky, I'm not sure there is a cleaner solution.

MaskRay added inline comments.Jun 29 2022, 9:54 PM
clang/lib/Basic/Targets/RISCV.h
142

There is a constructor Twine(char)

gbenyei marked 3 inline comments as done.Jun 30 2022, 8:25 AM
gbenyei added inline comments.
llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp
554 ↗(On Diff #440196)

Removed this part anyway - JIT is out of scope for this commit. Thanks.

gbenyei updated this revision to Diff 441410.Jun 30 2022, 8:29 AM

Removed LLD and JIT related parts - JIT is out of my scope, and LLD will be in an additional patch.
Fixed additional remarks.

asb added a comment.Jul 7 2022, 8:36 AM

Thanks for this patch Guy. As just discussed in the RISC-V sync-up call, it would be helpful from a review perspective to write down at least a simple plain-text description of the changes to the psABI doc needed to reflect the BE ABI implemented by GCC (and soon LLVM), perhaps in an issue.

I think this patch is lacking some test coverage around things like fixup handling (e.g. the logic to swap fixups).