This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Implement wave32 DWARF register mapping
ClosedPublic

Authored by RamNalamothu on Mar 18 2020, 6:29 AM.

Details

Summary

Implement the DWARF register mapping described in
llvm/docs/AMDGPUUsage.rst

This enables generating appropriate DWARF register numbers for
wave64 and wave32 modes.

Diff Detail

Event Timeline

RamNalamothu created this revision.Mar 18 2020, 6:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2020, 6:29 AM
arsenm added inline comments.Mar 18 2020, 7:25 AM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
253–255

Ternary operator

llvm/unittests/MC/AMDGPU/CMakeLists.txt
2

Missing the file with check for AMDGPU registered target? I'm not sure exactly how that works with unit tests, but it's done for GlobalISel unit tests with AArch64

llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp
20

probably needs call_once

31

Can this be a StringRef? You also don't reuse it below. This should probably also use the full 3 component triple

34

This should be an ASSERT_TRUE(t) << Error

39

ASSERT_TRUE

45

Don't need the llvm prefixes on all of these

91

Same as above

96

Same as above

llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp
28

ASSERT_TRUE(T) << Error

92

Should probably repeat test with more combinations of wavefront size subtarget features

Address review comments.

RamNalamothu marked 11 inline comments as done.Mar 18 2020, 9:03 AM
RamNalamothu added inline comments.
llvm/unittests/MC/AMDGPU/CMakeLists.txt
2

Sorry didn't get you and also I didn't notice anything special here with AArch64 unit tests.

llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp
31

Using only the 2 component triple as the dwarf register mappings are agnostic to a particular OS (the 3rd component).

arsenm added inline comments.Mar 18 2020, 10:19 AM
llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp
30–39

This common setup code should be moved to a setup function, and the test should not fail on target creation failure. Otherwise this will fail when AMDGPU isn't build. unittests/CodeGen/AArch64SelectionDAGTests.cpp is a model for this

31

The second part doesn't do anything. Setting the OS is the interesting part since it does default to the host OS, so introduces potential variability

Address review comments.

RamNalamothu marked 5 inline comments as done.Mar 19 2020, 12:43 AM
arsenm added inline comments.Mar 19 2020, 9:53 AM
llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp
45

This isn't creating an owning reference? Why is this unique ptr?

50

The multiple OS tests aren't needed, my point was it should just be set to something sensible other than the defaulted host OS

56

This can be a regular array

80

Regular array

Use array for test values and eliminate memory leak.

RamNalamothu marked 6 inline comments as done.Mar 19 2020, 12:32 PM
RamNalamothu added inline comments.
llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp
45

Thanks for catching this. Fixed the memory leak.

50

May be not needed but they do test few OS flavors for sanity and I guess there is no harm in having those tests.

arsenm accepted this revision.Mar 19 2020, 1:50 PM
arsenm added inline comments.
llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp
74

These can just be const char* arrays

This revision is now accepted and ready to land.Mar 19 2020, 1:50 PM
scott.linder accepted this revision.Mar 19 2020, 2:40 PM

Thanks Ram, LGTM too. Sorry for taking so long to review.