Page MenuHomePhabricator

[NVPTX] Initial adaptation of MCAsmStreamer/MCTargetStreamer for debug info in Cuda.
ClosedPublic

Authored by ABataev on Nov 14 2017, 9:17 AM.

Details

Summary

Initial changes in interfaces of MCAsmStreamer/MCTargetStreamer for
correct debug info emission for Cuda.

  1. PTX foramt does not support .ascii directives. Added the ability to

nullify it.

  1. DWARF .file directives must be emitted after .text section and

cannot be inlined in function bodies.

  1. The initial function label must follow the first debug .loc

directive, not be followed by.

  1. DWARF sections must be enclosed in braces.

Diff Detail

Repository
rL LLVM

Event Timeline

ABataev created this revision.Nov 14 2017, 9:17 AM
jlebar added a reviewer: tra.Dec 5 2017, 11:34 AM
echristo edited edge metadata.Dec 5 2017, 11:44 AM

This needs careful review. I'll take a look as soon as I can.

What do you mean by this:

"DWARF .loc directives must be emitted after .text section and
cannot be inlined in function bodies."

What do you mean by this:

"DWARF .loc directives must be emitted after .text section and
cannot be inlined in function bodies."

Sorry, my mistake. I mean .file directive.

ABataev edited the summary of this revision. (Show Details)Dec 5 2017, 11:59 AM
tra accepted this revision.Dec 5 2017, 12:00 PM

LGTM in general. Can you think of a way to verify new functionality? It looks like we may have to wait until some target starts using it.

lib/MC/MCAsmStreamer.cpp
1101 ↗(On Diff #122857)

We do not print EOL after the file name any more. Is that intentional?

lib/MC/MCStreamer.cpp
62–64 ↗(On Diff #122857)

Do you have further plans to do something more elaborate here? Otherwise I'd remove it and just use EmitRawText.

This revision is now accepted and ready to land.Dec 5 2017, 12:00 PM
tra added a comment.Dec 5 2017, 12:03 PM

I'll defer to @echristo for the final approval.

ABataev added inline comments.Dec 5 2017, 12:04 PM
lib/MC/MCAsmStreamer.cpp
1101 ↗(On Diff #122857)

EmitRawText emits EOL automatically.

lib/MC/MCStreamer.cpp
62–64 ↗(On Diff #122857)

Sure, I have plans. As soon as this patch is accepted, I'm going to publish next patches with debug info support for NVPTX. This is just the first part.

In D40033#945474, @tra wrote:

I'll defer to @echristo for the final approval.

Sure, that's why added Eric as a reviewer. :) Some changes may brake codegen for non-NVPTX targets and we have to be sure that everything works correctly everywhere.

Note that PTX allows .file even before .text. The spec is accurate here:

"The .file directive is allowed only in the outermost scope, i.e., at the same level as kernel and device function declarations."

The reason .file may be needed after .text is that if while emitting the .text, you do not know the file directives yet.

Note that PTX allows .file even before .text. The spec is accurate here:

"The .file directive is allowed only in the outermost scope, i.e., at the same level as kernel and device function declarations."

The reason .file may be needed after .text is that if while emitting the .text, you do not know the file directives yet.

Yes, you're right. That's the idea.

echristo added inline comments.Dec 5 2017, 12:57 PM
test/CodeGen/Mips/mips16ex.ll
6 ↗(On Diff #122857)

For the record this change is the one I'm concerned about. I don't think it's desired to put the cfi directives before the symbol.

That said, I'm going to have to take a closer look.

ABataev added inline comments.Dec 5 2017, 1:02 PM
test/CodeGen/Mips/mips16ex.ll
6 ↗(On Diff #122857)

Yes, I understand this. It is required in PTX to have .loc before $func_begin symbol, otherwise it emits incorrect debug info. At least, we can make this change only for Cuda.

ABataev added inline comments.Dec 14 2017, 6:12 AM
test/CodeGen/Mips/mips16ex.ll
6 ↗(On Diff #122857)

Eric, any news?

jlebar added inline comments.Dec 14 2017, 1:28 PM
test/CodeGen/Mips/mips16ex.ll
6 ↗(On Diff #122857)

Proxying ecirhsto for a moment, since he's currently slammed with something else: Your suggestion of doing this just for CUDA would resolve a lot of the fears here.

ABataev updated this revision to Diff 127124.Dec 15 2017, 7:05 AM

Removed emission of the function start label after first debug .loc directive. Will add it later with the test.

(Is this ready for someone to have another look?)

(Is this ready for someone to have another look?)

(I'm pretty motivated to help you get the reviews you need on this one and the other NVPTX debuginfo patches, so don't be shy to ping me here or directly over email if you're not getting movement from someone at Google. But FYI I'll be gone on vacation from Dec 24 to Jan 7.)

(Is this ready for someone to have another look?)

(I'm pretty motivated to help you get the reviews you need on this one and the other NVPTX debuginfo patches, so don't be shy to ping me here or directly over email if you're not getting movement from someone at Google. But FYI I'll be gone on vacation from Dec 24 to Jan 7.)

Are you ok with this patch? If so, I will commit it and start to prepare the next patches

jlebar accepted this revision.Dec 19 2017, 11:59 AM

Are you ok with this patch? If so, I will commit it and start to prepare the next patches

Judging by the fact that you don't have to change any tests here, it looks like this does not change debuginfo for any platforms. My understanding is this was echristo's main concern. LGTM.

This revision was automatically updated to reflect the committed changes.