[Lldb-commits] [PATCH] D128612: RISC-V big-endian support implementation

[Guy Benyei via Phabricator via lldb-commits]

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