[PATCH] D79545: [VE] Implements minimum MC layer for VE (3/4)
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 27 02:07:56 PDT 2020
jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.
Great work, LGTM. Thanks for persevering! The testing framework you've started should also prove very useful in the future, I think.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1464
ENUM_ENT(EM_BPF, "EM_BPF"),
+ ENUM_ENT(EM_VE, "NEC SX-Aurora Vector Engine"),
};
----------------
kaz7 wrote:
> 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.
Yeah, there might be a way to resolve that, but I'm not sure exactly how. I certainly think this format makes sense for the current context.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79545/new/
https://reviews.llvm.org/D79545
More information about the llvm-commits
mailing list