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

[Kazushi Marukawa via Phabricator via llvm-commits]

kaz7 marked 6 inline comments as done.
kaz7 added a comment.

Add some tests locally and found a problem need other's help.  Please let me know how to fix EM_ECOG1X problem and how to check Tripe::ve.  Thanks!



================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:1142-1143
       return "elf64-bpf";
+    case ELF::EM_VE:
+      return "elf64-ve";
     default:
----------------
jhenderson wrote:
> Test coverage?
Add one.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:1222-1223
 
+  case ELF::EM_VE:
+    return Triple::ve;
+
----------------
jhenderson wrote:
> Test coverage?
Is there any idea how to test it?  I've checked other tests and found llvm-ifs which I've never used.  Using llvm-ifs is correct way to implement test for this?  Thanks.


================
Comment at: llvm/lib/Object/ELF.cpp:148-154
+  case ELF::EM_VE:
+    switch (Type) {
+#include "llvm/BinaryFormat/ELFRelocs/VE.def"
+    default:
+      break;
+    }
+    break;
----------------
jhenderson wrote:
> Please add an llvm-readobj test "reloc-types-elf-ve.test" to test/tools/llvm-readobj/ELF similar to the existing "reloc-types-elf-*.test" files for other machines.
Thank you for examples.  I'll add a test.  Having unknown errors, ATM.


================
Comment at: llvm/lib/Object/ELF.cpp:201-202
     break;
+  case ELF::EM_VE:
+    return ELF::R_VE_RELATIVE;
   default:
----------------
jhenderson wrote:
> Test coverage?
I will try it in above "reloc-types-elf-*.test".


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:666-668
+  case ELF::EM_VE:
+#include "llvm/BinaryFormat/ELFRelocs/VE.def"
+    break;
----------------
jhenderson wrote:
> There should be some yaml2obj/obj2yaml testing to show that VE relocation enums are supported in those tools.
Thanks.  I'll try implement one.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1464
   ENUM_ENT(EM_BPF,           "EM_BPF"),
+  ENUM_ENT(EM_VE,            "NEC SX-Aurora Vector Engine"),
 };
----------------
jhenderson wrote:
> It looks like this enum is currently essentially untested. Could you add a simple test case to llvm-readobj's tests to cover file header printing of the different machine types, please?
> 
> Here's a rough example of what I'm thinking of:
> ```
> # RUN: yaml2obj %s -o %t.none -D MACHINE=EM_NONE
> # RUN: llvm-readelf --file-headers %t.none | FileCheck %s -D MACHINE=None --check-prefix=GNU
> 
> # RUN: yaml2obj %s -o %t.m32 -D MACHINE=EM_M32
> # RUN: llvm-readelf --file-headers %t.m32 | FileCheck %s -D MACHINE=WE32100
> 
> <repeat for each value>
> 
> # CHECK: Machine: [[MACHINE]]
> 
> --- !ELF
> FileHeader:
>   Class:   ELFCLASS32
>   Data:    ELFDATA2LSB
>   Type:    ET_REL
>   Machine: [[MACHINE]]
> ```
> 
> 
> 
I've made a test and found a problem that EM_ECOG1 and EM_ECOG1X uses identical number 168.  Let me know what I should do for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79545