This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] gfx1031 target
ClosedPublic

Authored by rampitec on Aug 5 2020, 12:21 PM.

Diff Detail

Event Timeline

rampitec created this revision.Aug 5 2020, 12:21 PM
Herald added a project: Restricted Project. · View Herald Transcript
rampitec requested review of this revision.Aug 5 2020, 12:21 PM
arsenm added inline comments.Aug 5 2020, 12:22 PM
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

rampitec added inline comments.Aug 5 2020, 12:26 PM
llvm/test/CodeGen/AMDGPU/llvm.amdgcn.atomic.csub.ll
2

Sure, although there are only two of them.

rampitec updated this revision to Diff 283335.Aug 5 2020, 12:27 PM
rampitec marked an inline comment as done.

Added global isel run lines.

arsenm accepted this revision.Aug 5 2020, 12:33 PM

LGTM with nit

clang/lib/Basic/Targets/AMDGPU.cpp
178

I would expect 1030 to come first

This revision is now accepted and ready to land.Aug 5 2020, 12:33 PM
rampitec added inline comments.Aug 5 2020, 12:35 PM
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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2020, 12:36 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jhenderson added inline comments.Aug 6 2020, 1:09 AM
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.

rampitec added inline comments.Aug 6 2020, 9:11 AM
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.

jhenderson added inline comments.Aug 10 2020, 1:07 AM
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:

  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).
rampitec marked 2 inline comments as done.Aug 10 2020, 1:00 PM
rampitec added inline comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
1844

Added in D85683.