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

[Craig Topper via Phabricator via lldb-commits]

craig.topper added inline comments.


================
Comment at: clang/lib/Basic/Targets/RISCV.h:111
     SizeType = UnsignedInt;
-    resetDataLayout("e-m:e-p:32:32-i64:64-n32-S128");
   }
----------------
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.


================
Comment at: lld/ELF/Arch/RISCV.cpp:160
+  if (config->is64) {
+    if (config->isLE)
+      write64le(buf, mainPart->dynamic->getVA());
----------------
Is it possible to use write64 instead of write64le/be? It looks like it checks the endianness internally.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:628
+  // For big endian cores, data fixup should be swapped.
+  bool swapValue = (Endian == support::big) && isDataFixup(Kind);
   for (unsigned i = 0; i != NumBytes; ++i) {
----------------
Capitalize `swapValue`


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h:34
                   const MCTargetOptions &Options)
-      : MCAsmBackend(support::little), STI(STI), OSABI(OSABI), Is64Bit(Is64Bit),
+      : MCAsmBackend(IsLittleEndian ? support::little : support::big), STI(STI), OSABI(OSABI), Is64Bit(Is64Bit),
         TargetOptions(Options) {
----------------
Is this longer than 80 columns?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCAsmInfo.cpp:24
+  if (TT.getArch() == Triple::riscv32be || TT.getArch() == Triple::riscv64be)
+    IsLittleEndian = false;
+
----------------
Could we do IsLittleEndian = TT.isLittleEndian()?


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:64
+      return "e-m:e-p:64:64-i64:64-i128:128-n64-S128";
+    else
+      return "E-m:e-p:64:64-i64:64-i128:128-n64-S128";
----------------
No else after return


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128612/new/

https://reviews.llvm.org/D128612