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

[Kazushi Marukawa via Phabricator via llvm-commits]

kaz7 planned changes to this revision.
kaz7 marked 5 inline comments as done.
kaz7 added a comment.

No problem.  Thank you for your time for reviewing.  I also almost ran out my time this month to work on this.  ;)



================
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:
> > kaz7 wrote:
> > > 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
> > FYI, I've checked registry status internally.  And I heard that it is registered through registry at sco.com.
> Great, good to hear. When I was referring to clang-formatting things, I was specifically referring to the new files you've added, rather than this enum, which I'm not convinced we should reformat. Could you undo the formatting changes to this bit, please?
Thanks.  This clang-format thing is useful, but it becomes little annoying if I modifies code region which doesn't follow clang-format style.  ;). Anyway, I'll revert this.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:30
+    if (Encoding == ELF::ELFDATA2MSB)
+      sys::swapByteOrder(Machine);
+
----------------
jhenderson wrote:
> If I'm reading the code correctly, this will unconditionally swap bytes for big endian inputs, regardless of the host endianness. If the host machine is big endian, you don't want to swap. There are build bots out there which are big endian, so you need to handle that case, or the test will fail on those.
> 
> See my out-of-line comment for a possible solution.
You are right.  I didn't think about that scenario.  And thank you for your example.


================
Comment at: llvm/unittests/Object/ELFTest.cpp:57
+}

----------------
jhenderson wrote:
> No need for an additional blank line here at the end of the file.
Thank you for mentioning that.  I also want to remove blank lines at the EOF and blanks at the EOL.  But this time I cannot find blank line at the end of this file.  Maybe smoking of glitches on the web?  Please let me know if you find a blank like again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79545