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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Two nits. Otherwise looks good. Your comment references llvm-objdump, but there's nothing testing llvm-objdump here. I imagine it's just a case of "it just works", due to the `Object` library changes?

I haven't checked that the LOONGARCH definitions match up with any official spec. Do you have anything you could cite for these values i.e. ELF generic-abi mailing list for the EM value, and some psABI doc for the relocations?

Not that I think this particular change needs it, but has there been any particular buy-in from the community for integrating this architecture at all? (In this case, I don't think it's necessary, but for wider changes, it probably is).

Please note that I won't be online until the 4th of January, after today's work, so may be slow to respond to anything.



================
Comment at: llvm/include/llvm/BinaryFormat/ELFRelocs/LoongArch.def:1
+
+#ifndef ELF_RELOC
----------------
I imagine that this is copied from other files, but this blank line can be deleted.

I'm not sure why we don't include a license header at the top here, but I presume that's the case in the other files too?


================
Comment at: llvm/include/llvm/BinaryFormat/ELFRelocs/LoongArch.def:45
+ELF_RELOC(R_LARCH_SOP_POP_32_S_0_5_10_16_S2,             44)
+ELF_RELOC(R_LARCH_SOP_POP_32_S_0_10_10_16_S2,            45)
+ELF_RELOC(R_LARCH_SOP_POP_32_U,                          46)
----------------
You can reduce the amount of spaces, to make it slightly easier to line things up. I've highlighted this line, as it's the longest, but you can then move all the other numbers to match up.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-loongarch64.test:1
+## Test that llvm-readobj/llvm-readelf shows proper relocation type
+## names and values for loongarch64 target.
----------------
You can drop "elf" from the title of this test, since it's already in the `ELF` subdirectory.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:295
+                                      "elf64-loongarch", "elf64-loongarch"};
+  std::array<Triple::ArchType, 4> Archs = {Triple::loongarch32, Triple::loongarch32,
+                                           Triple::loongarch64, Triple::loongarch64};
----------------
Please clang-format your newly added code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115859