This is an archive of the discontinued LLVM Phabricator instance.

[dwarfdump][AMDGPU] Support EF_AMDGPU_MACH_NONE
ClosedPublic

Authored by scott.linder on Feb 17 2023, 2:16 PM.

Details

Summary

A quirk of the AMDGPU backend is EF_AMDGPU_MACH_NONE, which is not
specific to the r600 or amdgcn architecture, but can be combined with
either.

AMDGPU ELF code objects with this mach value cannot be mapped back to a
Triple architecture, as it could be either r600 or amdgcn. For
llvm-dwarfdump this means the normal method of inspecting
ObjectFile::getArch to determine how to handle relocations is
insufficient.

This patch adds an extra check for ELF code objects which would
otherwise be categorized as UnknownArch, making it possible to use
llvm-dwarfdump with them. For completeness it also adds support for
known r600 machines.

This also adds specific tests for llvm-dwarfdump for amdgcn and r600,
both with known and unknown mach values.

Diff Detail

Event Timeline

scott.linder created this revision.Feb 17 2023, 2:16 PM
Herald added a project: Restricted Project. · View Herald Transcript
scott.linder requested review of this revision.Feb 17 2023, 2:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2023, 2:16 PM

The patch title and code are to do with llvm-dwarfdump, but the commit message refers to llvm-objdump. Is this just a mistake?

llvm/lib/Object/RelocationResolver.cpp
260

Nit

850–851

No need for else after return.

The patch title and code are to do with llvm-dwarfdump, but the commit message refers to llvm-objdump. Is this just a mistake?

Yes, just a typo, thank you!

scott.linder edited the summary of this revision. (Show Details)

Address feedback

It looks like this code is testing code in libObject, but the testing is in llvm-dwarfdump. Should the tests be moved to test/Object?

llvm/lib/Object/RelocationResolver.cpp
262

I think this function is a bit incorrectly named. Strictly-speaking, this will return true/false only based on the EM* value, so it doesn't say anything about whether the object is a "known" AMDGPU version. Perhaps should just be named isAMDGPUObject or similar?

llvm/test/tools/llvm-dwarfdump/AMDGPU/known-amdgcn-relocs.yaml
1 ↗(On Diff #498908)

I don't know whether this is a pattern you'll see widely in llvm-dwarfdump tests, but in newer tests I've been involved with, we've often used ## for comments, to distinguish them from the actual test logic.

7 ↗(On Diff #498908)

Have you looked into using the yaml2obj support for Dwarf? I haven't used it much, but you should see some examples in the yaml2obj testing on how to use it. It may or may not be sufficient for your testing purposes.

Alternatively, what does using YAML here give you above using the assembly directly in the test?

68 ↗(On Diff #498908)

I might be mistaken, but I believe this line is implicit, so can be omitted.

73 ↗(On Diff #498908)

Rather than just labelling these as "Test 1" etc, it might be worth stating why they are interesting cases.

130–141 ↗(On Diff #498908)

Nit: your indentation in the last line here is a little off, but in that case, you might as well remove the excessive spacing between key and value, so that there's just one space between the longest key and its value (and all the other values line up with each other).

llvm/test/tools/llvm-dwarfdump/AMDGPU/known-r600-relocs.yaml
1 ↗(On Diff #498908)

Same comments as above.

llvm/test/tools/llvm-dwarfdump/AMDGPU/unknown-amdgcn-relocs.yaml
1 ↗(On Diff #498908)

This test seems to be practically identical to the first one. Could you fold them into a separate test file and use yaml2obj's -D functionality to parameterise the YAML, so that you don't have to repeat it?

llvm/test/tools/llvm-dwarfdump/AMDGPU/unknown-r600-relocs.yaml
1 ↗(On Diff #498908)

Same comments as other unknown test.

scott.linder marked 7 inline comments as done.Mar 1 2023, 1:38 PM

It looks like this code is testing code in libObject, but the testing is in llvm-dwarfdump. Should the tests be moved to test/Object?

I can add more in test/Object, but the change was originally about providing a better experience in llvm-dwarfdump, so it seemed like the right place. There are no unittests of the RelocationResolver APIs that I can find, which would be the other appropriate place.

llvm/lib/Object/RelocationResolver.cpp
262

Agreed, I changed the name to isAMDGPU, but retained the exposition in the doc-comment.

llvm/test/tools/llvm-dwarfdump/AMDGPU/known-amdgcn-relocs.yaml
7 ↗(On Diff #498908)

I didn't realize it supported DWARF directly :^) thank you for the pointer

I moved everything into the YAML, let me know what you think

73 ↗(On Diff #498908)

The description would just repeat the contents of the test; there is nothing particularly interesting about any case, I just threw multiple variations at each relocation kind to get some coverage.

I just deleted the redundant "Test #" comments

scott.linder marked an inline comment as done.

Address feedback

No need to move the tests unless you want to. Your logic for where they are is as good as anything else.

llvm/test/tools/llvm-dwarfdump/AMDGPU/amdgcn-relocs.yaml
1

Does this test need renaming, since it covers R600 too?

17

Nit: I'd get rid of the extra blank line (but I would have one before the start of the YAML).

18–19

Up to you, but you could change these to be shared with the R600 cases too, since the message is the same.

That being said, why are you checking them?

138–141

Again, I'd get rid of the extra blank line, and then add one after the CHECK, but I don't feel strongly about it.

scott.linder marked 4 inline comments as done.Mar 6 2023, 12:25 PM
scott.linder added inline comments.
llvm/test/tools/llvm-dwarfdump/AMDGPU/amdgcn-relocs.yaml
18–19

AFAICT there are no other tests of llvm-dwarfdump with amdgpu code objects, so I wanted to capture as much of the current behavior as possible, including the warning about MCRegInfo that comes up when the object is an "Unknown" amdgpu arch. Ideally we would eventually have a more meaningful error message for the lay-user, or have some fallback behavior, but I didn't want to invest the time now.

scott.linder marked an inline comment as done.

Address feedback:

  • Fix test name to reflect it testing both r600 and amdcn
  • Common up some checks between r600 and amdgcn
  • Adjust some whitespace to make test more readable
jhenderson accepted this revision.Mar 7 2023, 12:37 AM

Looks good to me.

This revision is now accepted and ready to land.Mar 7 2023, 12:37 AM
This revision was automatically updated to reflect the committed changes.