[PATCH] D79545: [VE] Implements minimum MC layer for VE (3/4)

[Kazushi Marukawa via Phabricator via llvm-commits]

kaz7 marked 2 inline comments as done.
kaz7 added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-ve.test:83
+      - Type: R_VE_DTPOFF64
+#     - Type: R_VE_TPOFF64
+      - Type: R_VE_TLS_GD_HI32
----------------
jhenderson wrote:
> Rather than commenting these out, you might want to (if possible) use an explicit value for these ones, and then check that the value isn't interpreted, but rather treated as an unknown value. If you do that, I'd add a comment explaining what you are doing.
I see.  Listing commented out values here is strange as you explained.  This time I removed  those commented out values from tests.


================
Comment at: llvm/test/tools/obj2yaml/ELF/relocation-type-ve.yaml:1
+## Show how obj2yaml dumps relocation types.
+
----------------
jhenderson wrote:
> I don't think copying the existing `relocation-type.yaml` test is particularly useful. That is more for generic handling of relocation types, whereas we want to explicitly show that the VE relocation types are understood by yaml2obj and obj2yaml. Instead, I'd just enumerate the relocation values as you have in the llvm-readobj test. It's okay to explicitly specify them, because we know from the llvm-readobj testing that their values are correct.
Thanks.  I listed all types and test them using obj2yaml here similar to an above llvm-readobj test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79545