[PATCH] D115859: [LoongArch 2/5] Add ELF machine flag and relocs for upcoming LoongArch target

WÁNG Xuěruì via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Dec 19 23:26:34 PST 2021


xen0n added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/ELFRelocs/LoongArch.def:5
+
+// Based on the psABI: https://loongson.github.io/LoongArch-Documentation/LoongArch-ELF-ABI-EN.html
+// Adoption in binutils: include/elf/loongarch.h
----------------
It may be better to point to the documentation source code with exact commit hash, because unfortunately there seems to be no versioning in the rendered website, so future revisions to the documentation could render this comment out-dated.

Also minor nit: it should probably say "from the LoongArch ELF psABI", both because the values are not modified here downstream, and to fully spell out the documentation title.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:292
 
+TEST(ELFObjectFileTest, MachineTestForLOONGARCH) {
+  std::array<StringRef, 4> Formats = {"elf32-loongarch", "elf32-loongarch",
----------------
`MachineTestForLoongArch` for proper casing?


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:293
+TEST(ELFObjectFileTest, MachineTestForLOONGARCH) {
+  std::array<StringRef, 4> Formats = {"elf32-loongarch", "elf32-loongarch",
+                                      "elf64-loongarch", "elf64-loongarch"};
----------------
>From my cursory look, it seems the 2nd and 4th elements are for verifying big-endian cases, yet LoongArch is little-endian-only according to documentation. So do you want to ignore big-endian cases and fix here to say `elfNN-unknown` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115859



More information about the llvm-commits mailing list