This is an archive of the discontinued LLVM Phabricator instance.

[DEBUGINFO, NVPTX] Disable emission of ',debug' option if only debug directives are allowed.
ClosedPublic

Authored by ABataev on Apr 25 2018, 7:03 AM.

Diff Detail

Event Timeline

ABataev created this revision.Apr 25 2018, 7:03 AM
ABataev updated this revision to Diff 158619.Aug 1 2018, 1:33 PM

Updated and reworked to latest changes

Do you mean basically just line tables or something else? (Minus the need in normal dwarf for a minimal cu to associate the line table with). Also, how does this handle inlining etc?

Do you mean basically just line tables or something else? (Minus the need in normal dwarf for a minimal cu to associate the line table with). Also, how does this handle inlining etc?

When we have full debug info, we need to add debug option to the target directive. When we do not emit debug info or emit just debug directives (i.e. only lineinfo), this options should not be added to the target directive. Plus we need to render all the debug directives (.line|.file) to be able to get the correct data from the profiler.
The inlining is handled only in terms of files/location debug directives, nothing else.

ABataev updated this revision to Diff 172164.Nov 1 2018, 10:22 AM

Removed an option, used emission kind for the check of the full debug info.

probinson accepted this revision.Nov 1 2018, 12:44 PM

LGTM

test/DebugInfo/NVPTX/debug-file-loc-only.ll
3 ↗(On Diff #172164)

Typo: "Bitcode in this test..." (not "int").

This revision is now accepted and ready to land.Nov 1 2018, 12:44 PM
This revision was automatically updated to reflect the committed changes.

Looks like Eric still had some questions here? Or at least hadn't commented
about whether the answers/changes addressed his concerns?

Looks like Eric still had some questions here? Or at least hadn't commented
about whether the answers/changes addressed his concerns?

My bad. It looked answered to me, but yes should have waited on his confirmation.

Some inline comments - I'm still not happy with some of how this patch is and would like to see some changes and elaborations of how we split things out. Mostly it's bikeshed naming things, but the current state is a bit more confusing than without.

Thanks!

-eric

llvm/trunk/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
888–890 ↗(On Diff #172350)

This seems to conflict with what you said above:

"When we have full debug info, we need to add debug option to the target directive. When we do not emit debug info or emit just debug directives (i.e. only lineinfo),"

as LineTablesOnly is line tables. If we need to split line tables into "things that handle inlining and things that don't" we should handle that discrepancy, but DebugDirectivesOnly isn't very helpful as a descriptor of what the compiler should be doing and paying attention to.

956–958 ↗(On Diff #172350)

What's going on here?

ABataev reopened this revision.Nov 9 2018, 8:39 AM
This revision is now accepted and ready to land.Nov 9 2018, 8:39 AM
ABataev updated this revision to Diff 173348.Nov 9 2018, 8:39 AM

Update after revert.

ABataev updated this revision to Diff 173351.Nov 9 2018, 8:44 AM

Removed extra functionality from the patch.

ABataev updated this revision to Diff 173352.Nov 9 2018, 8:46 AM

Fixed test comment.

ABataev added inline comments.Nov 9 2018, 8:52 AM
llvm/trunk/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
888–890 ↗(On Diff #172350)

Yes, because I reworked the patch for the frontend. LineTablesOnly is considered as normal debug info option now. Plus, even before, when we saw -line-table-only option for NVPTX target, we still emitted DICompileUnit::DebugDirectivesOnly debug info kind. So, this is ok even for the previous description.

956–958 ↗(On Diff #172350)

I split this patch into 2 different patches.

echristo accepted this revision.Dec 5 2018, 10:35 PM
This revision was automatically updated to reflect the committed changes.