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

[Jessica Clarke via Phabricator via lldb-commits]

jrtc27 added inline comments.


================
Comment at: clang/lib/Basic/Targets/RISCV.h:113
+    if (Triple.isLittleEndian())
+      resetDataLayout("e-m:e-p:32:32-i64:64-n32-S128");
+    else
----------------
And please avoid repeating the whole data layout, just make the e/E a variable


================
Comment at: clang/lib/Driver/ToolChains/FreeBSD.cpp:235
+    CmdArgs.push_back("-m");
+    CmdArgs.push_back("elf32briscv");
+    break;
----------------
-X to match LE. Or ditch these (and I can ditch riscv32...) since FreeBSD only supports riscv64.


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1700
                 {M.gccSuffix(),
                  "/../../../../riscv64-unknown-elf/lib" + M.gccSuffix(),
                  "/../../../../riscv32-unknown-elf/lib" + M.gccSuffix()});
----------------
Just shove a `+ Be +` in the middle of these two rather than introducing a whole new set and picking between them?


================
Comment at: llvm/cmake/config-ix.cmake:463
   set(LLVM_NATIVE_ARCH RISCV)
+elseif (LLVM_NATIVE_ARCH MATCHES "riscv32be")
+  set(LLVM_NATIVE_ARCH RISCV)
----------------
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?


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:554
         .buildGraph();
-  } else {
-    assert((*ELFObj)->getArch() == Triple::riscv32 &&
-           "Invalid triple for RISCV ELF object file");
+  } else if ((*ELFObj)->getArch() == Triple::riscv64be) {
+    auto &ELFObjFile = cast<object::ELFObjectFile<object::ELF64BE>>(**ELFObj);
----------------
Why switch to this order when before you've used 32, 64, 32be, 64be as the order


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:65
+
+    return "E-m:e-p:64:64-i64:64-i128:128-n64-S128";
+  }
----------------
As with Clang


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128612