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

Mon May 11 01:34:21 PDT 2020

jhenderson added inline comments.

================
Comment at: llvm/include/llvm/BinaryFormat/ELFRelocs/VE.def:36
+ELF_RELOC(R_VE_DTPOFF64,     23)
+// ELF_RELOC(R_VE_TPOFF64,     24)
+ELF_RELOC(R_VE_TLS_GD_HI32,  25)
----------------
kaz7 wrote:
> jhenderson wrote:
> > Why are some commented out?
> I left them commented out because documents left them as commented out.  This time, I put everything defined in manual.  I can leave items what I use.  In order to implement test for all of them, I have to do so...
> 
> https://www.hpc.nec/documents/guide/pdfs/VE-ABI_v2.1.pdf   section 4.4.1
> https://www.hpc.nec/documents/guide/pdfs/VE-tls_v1.2.pdf   section 5.1
That's a slightly weird (to me) way of omitting certain relocation types in the manual, but okay, that's fine. I'm happy that they remain commented out then.

================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:1222-1223

+  case ELF::EM_VE:
+    return Triple::ve;
+
----------------
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.

================
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
----------------
Please explain (with a comment) what this test is trying to test. Why "unknown" in the title?

================
Comment at: llvm/test/Object/VE/elf64-unknown.yaml:2
+# RUN: yaml2obj %s -o %t.o
+# RUN: llvm-readobj -S --file-headers %t.o | FileCheck %s
+
----------------
Why `-S`?

================
Comment at: llvm/test/tools/llvm-readobj/ELF/machine-enum.test:1
+# Test all machine code whether correctly handled.
+# 
----------------
Thanks for the test.

Please use '##' for comments, to help them stand out better. Also I'd rephrase this comment to be more grammatically correct:

`## Show that all machine codes are correctly printed.`

================
Comment at: llvm/test/tools/llvm-readobj/ELF/machine-enum.test:2
+# Test all machine code whether correctly handled.
+# 
+# RUN: yaml2obj %s -o %t.o -D MACHINE=EM_NONE
----------------
Delete the '#' from this line, please.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/machine-enum.test:3-4
+# 
+# RUN: yaml2obj %s -o %t.o -D MACHINE=EM_NONE
+# RUN: llvm-readelf --file-headers %t.o | FileCheck %s -DMACHINE="None"
+
----------------
As a suggestion, it might be better to name each object differently, i.e. something like `%tnone`, `%tm32` etc. That way it'll be easier to investigate a test failure.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-ve.test:53
+    Type:    SHT_PROGBITS
+    Content: 00
+  - Name:         .rela.text
----------------
You don't need this Content.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-ve.test:56-57
+    Type:         SHT_RELA
+    AddressAlign: 0x0000000000000008
+    EntSize:      0x0000000000000018
+    Info:         .text
----------------
You don't need these two fields.

================
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
----------------
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.

================
Comment at: llvm/test/tools/obj2yaml/ELF/relocation-type-ve.yaml:1
+## Show how obj2yaml dumps relocation types.
+
----------------
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.

================
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:
> > 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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79545