Details
- Reviewers
msearles kzhuravl jhenderson arsenm - Commits
- rGea7d0e2996ec: [AMDGPU] gfx1031 target
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/test/CodeGen/AMDGPU/llvm.amdgcn.atomic.csub.ll | ||
---|---|---|
2 | Can you also add the equivalent run lines to the GlobalISel versions of the tests |
llvm/test/CodeGen/AMDGPU/llvm.amdgcn.atomic.csub.ll | ||
---|---|---|
2 | Sure, although there are only two of them. |
clang/lib/Basic/Targets/AMDGPU.cpp | ||
---|---|---|
178 | We always do it like this. The reason is a higher arch can add some features and then we fall-through. |
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
---|---|---|
1844 | 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. |
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
---|---|---|
1844 | It is in the lvm/test/CodeGen/AMDGPU/elf-header-flags-mach.ll above along with all other targets. |
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
---|---|---|
1844 | 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:
|
I would expect 1030 to come first