This is an archive of the discontinued LLVM Phabricator instance.

[CUDA][HIP] Fix -gsplit-dwarf option
ClosedPublic

Authored by yaxunl on Sep 16 2020, 1:27 PM.

Details

Summary

when -gsplit option is used with clang driver, clang driver will create
a filename with .dwo option based on the input file name and pass
it to clang -cc1. This file is used for storing the debug info. Since
CUDA/HIP generate separate object files for different GPU arch's,
this file should be different for different GPU arch. This patch
adds _ and GPU arch to the stem of the dwo file.

Diff Detail

Event Timeline

yaxunl requested review of this revision.Sep 16 2020, 1:27 PM
yaxunl created this revision.
MaskRay retitled this revision from [CUDA][HIP] Fix -gsplit option to [CUDA][HIP] Fix -gsplit-dwarf option.Sep 16 2020, 1:39 PM
tra accepted this revision.Sep 16 2020, 2:23 PM

Does this naming scheme the same as used for .o files? We may want to keep them in sync.

Other than that, LGTM.

clang/lib/Driver/ToolChains/CommonArgs.cpp
909

I think the same approach would make sense for CUDA, too.

This revision is now accepted and ready to land.Sep 16 2020, 2:23 PM
yaxunl marked an inline comment as done.Sep 16 2020, 7:54 PM
In D87791#2277887, @tra wrote:

Does this naming scheme the same as used for .o files? We may want to keep them in sync.

Other than that, LGTM.

.o file is different story.

For -fno-gpu-rdc, the .o files for device compilation are temporary files which are deleted after the device ISA are generated and embedded in host .o file. There is only one output .o file which is the host object file.

For -fgpu-rdc, the .o files for device compilation are also temporary files which are bundled into the clang-offload-bundle. There is only one output .o file which is a bundle.

Therefore in either case there is no need to rename the intermediate .o files since they are temporary files which have unique names.

The .dwo files are not temporary files. They are supposed to be shipped with .o files for debugging info.

Since .dwo files are not temporary files, it is not necessary to follow the -save-temps name convention. For the host object, we keep the original .dwo file name. For the device object, we add '_' and GPU arch to the stem, which is sufficient and concise.

clang/lib/Driver/ToolChains/CommonArgs.cpp
909

will include OFK_CUDA.

tra added a comment.Sep 17 2020, 10:54 AM

Therefore in either case there is no need to rename the intermediate .o files since they are temporary files which have unique names.

The .dwo files are not temporary files. They are supposed to be shipped with .o files for debugging info.

Ack.
BTW, is split-dwarf useful for AMD GPUs on device side? I don't think we can currently utilize DWO files on device side with CUDA at all. To think of it, it's probably going to break GPU-side debugging as CUDA can only deal with dwarf info embedded in the GPU binary.
If it does not work for AMD GPUs, perhaps we should just disable it for GPUs.

Since .dwo files are not temporary files, it is not necessary to follow the -save-temps name convention. For the host object, we keep the original .dwo file name. For the device object, we add '_' and GPU arch to the stem, which is sufficient and concise.

What will happen with -save-temps ? Will dwo files match object file names?

yaxunl marked an inline comment as done.Sep 17 2020, 11:12 AM
In D87791#2279821, @tra wrote:

Therefore in either case there is no need to rename the intermediate .o files since they are temporary files which have unique names.

The .dwo files are not temporary files. They are supposed to be shipped with .o files for debugging info.

Ack.
BTW, is split-dwarf useful for AMD GPUs on device side? I don't think we can currently utilize DWO files on device side with CUDA at all. To think of it, it's probably going to break GPU-side debugging as CUDA can only deal with dwarf info embedded in the GPU binary.
If it does not work for AMD GPUs, perhaps we should just disable it for GPUs.

It is requested by our debugger team, so it should work with amdgpu.

Since .dwo files are not temporary files, it is not necessary to follow the -save-temps name convention. For the host object, we keep the original .dwo file name. For the device object, we add '_' and GPU arch to the stem, which is sufficient and concise.

What will happen with -save-temps ? Will dwo files match object file names?

with -save-temps, the saved temporary files will be like test-hip-amdgcn-amd-amdhsa-gfx906.o. The dwo file will be like test_gfx906.dwo.

tra added a comment.Sep 17 2020, 11:26 AM

It is requested by our debugger team, so it should work with amdgpu.

Is the naming scheme for GPU-side DWO files dictated by debugger? If that's the case, it may be worth adding a comment about that.

LGTM.

(Note that ideally -gsplit-dwarf should not imply -g2 but it currents does so. And Clang and GCC have not agreed whether we should add a new flag like -fsplit-dwarf. /For -gsplit-dwarf builds, it is the best to ensure -g is also specified/.)

clang/lib/Driver/ToolChains/CommonArgs.cpp
918

Does stem change the semantics?

yaxunl marked an inline comment as done.Sep 17 2020, 11:40 AM
In D87791#2279885, @tra wrote:

It is requested by our debugger team, so it should work with amdgpu.

Is the naming scheme for GPU-side DWO files dictated by debugger? If that's the case, it may be worth adding a comment about that.

LGTM.

It is the preferred naming by the debugger. Will add a comment.

clang/lib/Driver/ToolChains/CommonArgs.cpp
918

No. replace_extension will take stem first and then add new extension.

This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2020, 7:07 AM