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"; + ---------------- MaskRay wrote: > 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128612/new/ https://reviews.llvm.org/D128612 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
