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

[Lu Weining via Phabricator via llvm-commits]

SixWeining added a comment.





================
Comment at: llvm/include/llvm/BinaryFormat/ELFRelocs/LoongArch.def:1
+
+#ifndef ELF_RELOC
----------------
jhenderson wrote:
> 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?
1. OK, I will delete the first blank line.
2. About the license header absence I think maybe the reason is this file is not a 'standalone' source or header file but a file that would be included by other files. 


================
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)
----------------
jhenderson wrote:
> 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.
No problem. I will change that.


================
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.
----------------
jhenderson wrote:
> You can drop "elf" from the title of this test, since it's already in the `ELF` subdirectory.
Good suggestion. But we just follow other existing files naming method in the same directory. Maybe we can change all these files in a seperate patch later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115859