[PATCH] D85337: [AMDGPU] gfx1031 target

Mon Aug 10 13:00:03 PDT 2020

rampitec marked 2 inline comments as done.
rampitec added inline comments.

================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1844
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX1030),
+  LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX1031),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_XNACK),
----------------
jhenderson wrote:
> rampitec wrote:
> > jhenderson wrote:
> > > Where's the dedicated llvm-readobj test for this? Please add one. If there exists one for the other AMDGPU flags, extend that one, otherwise, it might be best to write one for the whole set at once.
> > It is in the lvm/test/CodeGen/AMDGPU/elf-header-flags-mach.ll above along with all other targets.
> I emphasise "dedicated". llvm/test/CodeGen... is not the place for tests for llvm-readobj code. Such tests belong in llvm/test/tools/llvm-readobj.
> 
> To me, the CodeGen test looks like a test of the llc behaviour of generating the right value. It uses llvm-readobj, and therefore only incidentally tests the dumping behaviour. Imagine a situation where we decided to add support for this to llvm-objdump, and then somebody decides to switch the test over to using llvm-objdump to match (maybe it "looks better" that way). That would result in this code in llvm-readobj being untested.
> 
> My opinion, and one I think is shared with others who maintain llvm-readobj (amongst other things) is that you need direct testing for llvm-readobj behaviour within llvm-readobj tests to avoid such situations. This would therefore mean that this change needs two tests:
> 
> 1) A test in llvm/test/tools/llvm-readobj/ELF which uses a process (e.g. yaml2obj) to create the ELF header with the required flag, to ensure the dumping behaviour is correct.
> 2) A test in llvm/test/CodeGen which tests that llc produces the right output value in the ELF header flags field (which of course would use llvm-readobj currently, but could use anything to verify).
Added in D85683.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85337