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

[Kazushi Marukawa via Phabricator via llvm-commits]

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

Thank you for kind suggestions.



================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:1222-1223
 
+  case ELF::EM_VE:
+    return Triple::ve;
+
----------------
jhenderson wrote:
> kaz7 wrote:
> > kaz7 wrote:
> > > 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.
> > Not sure about llvm-ifs.  Please advise me.  Thanks.
> I don't know anything about llvm-ifs, but it looks like llvm-objdump or llvm-readobj --file-headers will do the job for you (the "architecture" field is derived from the object file's architecture).
> 
> I would expect to see a corresponding change to Triple.cpp's `getArchTypeName` though.
Thanks.  I could make a certain test for this using llvm-readobj.


================
Comment at: llvm/test/Object/VE/elf64-unknown.yaml:1
+# RUN: yaml2obj %s -o %t.o
+# RUN: llvm-readobj -S --file-headers %t.o | FileCheck %s
----------------
jhenderson wrote:
> Please explain (with a comment) what this test is trying to test. Why "unknown" in the title?
Thank you for letting me correct it.  I simply copied from others like ../AMDGPU/elf64-unknown.yaml.  I renamed this to elf64-machine.yaml.


================
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:
> kaz7 wrote:
> > 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.
> Just test what is printed for one of them. It might be a bug in LLVM, or a gABI bug or something similar. Speaking of the gABI, have you been officially assigned a value for `EM_VE` using the ELF gABI mailing list?
Ok.  I test onl EM_ECOG1.  Tests for ECOG1X is commented out in above test with FIXME tag.

Regarding to gABI, I don't know.  I guess EM_VE is officially assigned since EM_VE is used in NEC's official compiler systems like below.  I tried to check it out, but I didn't find the way to do so unfortunately.

https://github.com/veos-sxarr-NEC/musl-libc-ve/blob/master/include/elf.h#L220


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79545