[PATCH] D85337: [AMDGPU] gfx1031 target
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D85337: [AMDGPU] gfx1031 target
jhenderson 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), 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). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85337/new/ https://reviews.llvm.org/D85337 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D85337: [AMDGPU] gfx1031 target
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: > 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85337/new/ https://reviews.llvm.org/D85337 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D85337: [AMDGPU] gfx1031 target
This revision was automatically updated to reflect the committed changes. Closed by commit rGea7d0e2996ec: [AMDGPU] gfx1031 target (authored by rampitec). Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85337/new/ https://reviews.llvm.org/D85337 Files: clang/include/clang/Basic/Cuda.h clang/lib/Basic/Targets/AMDGPU.cpp clang/lib/Basic/Targets/NVPTX.cpp clang/test/CodeGenOpenCL/amdgpu-features.cl clang/test/Driver/amdgpu-macros.cl clang/test/Driver/amdgpu-mcpu.cl llvm/docs/AMDGPUUsage.rst llvm/include/llvm/BinaryFormat/ELF.h llvm/include/llvm/Support/TargetParser.h llvm/lib/ObjectYAML/ELFYAML.cpp llvm/lib/Support/TargetParser.cpp llvm/lib/Target/AMDGPU/GCNProcessors.td llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-fmad.s32.mir llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.global.atomic.csub.ll llvm/test/CodeGen/AMDGPU/elf-header-flags-mach.ll llvm/test/CodeGen/AMDGPU/hsa-note-no-func.ll llvm/test/CodeGen/AMDGPU/idot8s.ll llvm/test/CodeGen/AMDGPU/llvm.amdgcn.atomic.csub.ll llvm/test/CodeGen/AMDGPU/llvm.amdgcn.sdot4.ll llvm/test/CodeGen/AMDGPU/llvm.amdgcn.sdot8.ll llvm/test/MC/AMDGPU/gfx1030_err.s llvm/test/MC/AMDGPU/gfx1030_new.s llvm/test/MC/Disassembler/AMDGPU/gfx1030_dasm_new.txt llvm/tools/llvm-readobj/ELFDumper.cpp Index: llvm/tools/llvm-readobj/ELFDumper.cpp === --- llvm/tools/llvm-readobj/ELFDumper.cpp +++ llvm/tools/llvm-readobj/ELFDumper.cpp @@ -1841,6 +1841,7 @@ LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX1011), LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX1012), 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), LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_SRAM_ECC) }; Index: llvm/test/MC/Disassembler/AMDGPU/gfx1030_dasm_new.txt === --- llvm/test/MC/Disassembler/AMDGPU/gfx1030_dasm_new.txt +++ llvm/test/MC/Disassembler/AMDGPU/gfx1030_dasm_new.txt @@ -1,4 +1,5 @@ # RUN: llvm-mc -arch=amdgcn -mcpu=gfx1030 -disassemble -show-encoding < %s | FileCheck -check-prefix=GFX10 %s +# RUN: llvm-mc -arch=amdgcn -mcpu=gfx1031 -disassemble -show-encoding < %s | FileCheck -check-prefix=GFX10 %s # GFX10: global_load_dword_addtid v1, s[2:3] offset:16 0x10,0x80,0x58,0xdc,0x00,0x00,0x02,0x01 Index: llvm/test/MC/AMDGPU/gfx1030_new.s === --- llvm/test/MC/AMDGPU/gfx1030_new.s +++ llvm/test/MC/AMDGPU/gfx1030_new.s @@ -1,4 +1,5 @@ // RUN: llvm-mc -arch=amdgcn -mcpu=gfx1030 -show-encoding %s | FileCheck --check-prefix=GFX10 %s +// RUN: llvm-mc -arch=amdgcn -mcpu=gfx1031 -show-encoding %s | FileCheck --check-prefix=GFX10 %s global_load_dword_addtid v1, s[2:3] offset:16 // GFX10: encoding: [0x10,0x80,0x58,0xdc,0x00,0x00,0x02,0x01] Index: llvm/test/MC/AMDGPU/gfx1030_err.s === --- llvm/test/MC/AMDGPU/gfx1030_err.s +++ llvm/test/MC/AMDGPU/gfx1030_err.s @@ -1,4 +1,5 @@ // RUN: not llvm-mc -arch=amdgcn -mcpu=gfx1030 -show-encoding %s 2>&1 | FileCheck --check-prefix=GFX10 %s +// RUN: not llvm-mc -arch=amdgcn -mcpu=gfx1031 -show-encoding %s 2>&1 | FileCheck --check-prefix=GFX10 %s v_dot8c_i32_i4 v5, v1, v2 // GFX10: error: Index: llvm/test/CodeGen/AMDGPU/llvm.amdgcn.sdot8.ll === --- llvm/test/CodeGen/AMDGPU/llvm.amdgcn.sdot8.ll +++ llvm/test/CodeGen/AMDGPU/llvm.amdgcn.sdot8.ll @@ -3,6 +3,7 @@ ; RUN: llc -march=amdgcn -mcpu=gfx1011 -verify-machineinstrs < %s | FileCheck %s --check-prefixes=GCN,GFX10 ; RUN: llc -march=amdgcn -mcpu=gfx1012 -verify-machineinstrs < %s | FileCheck %s --check-prefixes=GCN,GFX10 ; RUN: llc -march=amdgcn -mcpu=gfx1030 -verify-machineinstrs < %s | FileCheck %s --check-prefixes=GCN,GFX10 +; RUN: llc -march=amdgcn -mcpu=gfx1031 -verify-machineinstrs < %s | FileCheck %s --check-prefixes=GCN,GFX10 declare i32 @llvm.amdgcn.sdot8(i32 %a, i32 %b, i32 %c, i1 %clamp) Index: llvm/test/CodeGen/AMDGPU/llvm.amdgcn.sdot4.ll === --- llvm/test/CodeGen/AMDGPU/llvm.amdgcn.sdot4.ll +++ llvm/test/CodeGen/AMDGPU/llvm.amdgcn.sdot4.ll @@ -2,6 +2,7 @@ ; RUN: llc -march=amdgcn -mcpu=gfx1011 -verify-machineinstrs < %s | FileCheck %s --check-prefixes=GCN,GFX10 ; RUN: llc -march=amdgcn -mcpu=gfx1012 -verify-machineinstrs < %s | FileCheck %s --check-prefixes=GCN,GFX10 ; RUN: llc -march=amdgcn -mcpu=gfx1030 -verify-machineinstrs < %s | FileCheck %s --check-prefixes=GCN,GFX10 +; RUN: llc -march=amdgcn -mcpu=gfx1031 -verify-machineinstrs < %s | FileCheck %s --check-prefixes=
[PATCH] D85337: [AMDGPU] gfx1031 target
jhenderson 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), 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85337/new/ https://reviews.llvm.org/D85337 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
