This is an archive of the discontinued LLVM Phabricator instance.

[DEBUG_INFO, NVPTX] Fix relocation info.
ClosedPublic

Authored by ABataev on Apr 18 2018, 11:56 AM.

Details

Summary

Initial function labels must follow the debug location for the correct relocation info generation.

Diff Detail

Event Timeline

ABataev created this revision.Apr 18 2018, 11:56 AM
tra accepted this revision.Apr 19 2018, 10:50 AM
This revision is now accepted and ready to land.Apr 19 2018, 10:50 AM
ABataev updated this revision to Diff 157679.Jul 27 2018, 6:49 AM

Updated to the latest revision.

Can you explain this a bit more with some examples of wrong relocations?

Thanks!

-eric

Can you explain this a bit more with some examples of wrong relocations?

Thanks!

-eric

It is the requirement of the ptxas. The first function label must follow the debug location, otherwise ptxas is unable to generate correct relo info for functions.

But you have other locations inside of functions so I don't get it.

-eric

But you have other locations inside of functions so I don't get it.

eric

Ptxas generates the relo info basing on the first label of the function. And it is unable to get correct debug location if there is no debug loc before the initial label. I don't know, how ptxas works, but I know that we had troubles with this when there were no debug loc before label.

nvcc always emits debug location before the very first label that represents function start. So, we can consider this as the requirement for NVPTX.

ABataev updated this revision to Diff 158831.Aug 2 2018, 1:40 PM

Reworked according to Eric's comments

ABataev updated this revision to Diff 159009.Aug 3 2018, 7:34 AM

Reworked the patch to change the way we get file id to make it more robust.

Rather than testing for isNVPTX in AsmPrinter.cpp I'd rather just make a function "emitPreFunctionDebugInfo" and have it do nothing unless it's NVPTX and then define this in the nvptx backend. Easier to update if nvidia ever fixes this weirdness too.

Rather than testing for isNVPTX in AsmPrinter.cpp I'd rather just make a function "emitPreFunctionDebugInfo" and have it do nothing unless it's NVPTX and then define this in the nvptx backend. Easier to update if nvidia ever fixes this weirdness too.

Seems to me you were looking to the very first patch. The last one looks exactly like you want. Switch to the latest revision.

ABataev updated this revision to Diff 172389.Nov 2 2018, 10:07 AM

Restored original recordSourceLine member function and introduced static recordSourceLine function.

Rather than testing for isNVPTX in AsmPrinter.cpp I'd rather just make a function "emitPreFunctionDebugInfo" and have it do nothing unless it's NVPTX and then define this in the nvptx backend. Easier to update if nvidia ever fixes this weirdness too.

Seems to me you were looking to the very first patch. The last one looks exactly like you want. Switch to the latest revision.

Closer, I think you can do all of this in the NVPTX backend by just having something emit an "empty" line directive where you want rather than needing to redo the target independent code. Just call recordSourceLine with bogus information in the backend? Do you need a real location or can something that looks like the line number program initial state work for you? If you need a valid one then it's possible you could run into problems with multiple cus in a single input file (which I agree is less than likely with cuda, but I don't want to discount the possibility of people linking cuda modules together).

Thoughts?

-eric

Rather than testing for isNVPTX in AsmPrinter.cpp I'd rather just make a function "emitPreFunctionDebugInfo" and have it do nothing unless it's NVPTX and then define this in the nvptx backend. Easier to update if nvidia ever fixes this weirdness too.

Seems to me you were looking to the very first patch. The last one looks exactly like you want. Switch to the latest revision.

Closer, I think you can do all of this in the NVPTX backend by just having something emit an "empty" line directive where you want rather than needing to redo the target independent code. Just call recordSourceLine with bogus information in the backend? Do you need a real location or can something that looks like the line number program initial state work for you? If you need a valid one then it's possible you could run into problems with multiple cus in a single input file (which I agree is less than likely with cuda, but I don't want to discount the possibility of people linking cuda modules together).

Thoughts?

-eric

  1. I cannot call DwarfDebug::recordSourceLine from the NVPTX backend, as it has no direct access to the DwarfDebug. It is accessible only from AsmPrinter. So, we need AsmPrinter::emitInitialRawDwarfLocDirective or something similar.
  2. Yes, we need the valid debug location here. Otherwise, it may cause incorrect data emission from the Cuda profiling tools. That's why we need the same functionality as the regular debug info has. I don't think there's going to be some problems with the multiple cus. The debug directives for NVPTX are quite primitive, they include just file number, line number + column position. The .file directives are emitted, so, I don't see why we may have troubles here.
ABataev updated this revision to Diff 176995.Dec 6 2018, 9:30 AM

Updated to the latest version.

ABataev updated this revision to Diff 181548.Jan 14 2019, 6:37 AM

Updated to latest version.

echristo accepted this revision.Jan 22 2019, 8:52 AM

The way this works is feeling pretty awkward, but I think updating that will be small in contrast to getting debug working for nvptx.

Thanks for the patience.

This revision was automatically updated to reflect the committed changes.