[PATCH] D128612: RISC-V big-endian support implementation
Guy Benyei via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 29 21:38:52 PDT 2022
gbenyei marked 3 inline comments as done.
gbenyei added a comment.
In D128612#3620911 <https://reviews.llvm.org/D128612#3620911>, @MaskRay wrote:
> In D128612#3618167 <https://reviews.llvm.org/D128612#3618167>, @gbenyei wrote:
>> In D128612#3617955 <https://reviews.llvm.org/D128612#3617955>, @MaskRay wrote:
>>> 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 <https://reviews.llvm.org/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.
Comment at: clang/lib/Basic/Targets/RISCV.h:144
+ StringRef LayoutEndianness = Triple.isLittleEndian() ? "e" : "E";
> You may use a `char` and possibly fold this into the expression below.
Concatenating a conditional char and a string literal might be tricky, I'm not sure there is a cleaner solution.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits