This is an archive of the discontinued LLVM Phabricator instance.

[AsmPrinter] Collapse .loc 0 0 directives
ClosedPublic

Authored by JDevlieghere on Jan 15 2019, 6:10 PM.

Details

Summary

Currently we do not always collapse subsequent .loc 0 0 directives. The reason is that we were checking for a PrevInstLoc which is not set when we emit a line-0 record. We should only check the LastAsmLine, which seems to be created exactly for this purpose.

// When we emit a line-0 record, we don't update PrevInstLoc; so look at
// the last line number actually emitted, to see if it was line 0.
unsigned LastAsmLine =
   Asm->OutStreamer->getContext().getCurrentDwarfLoc().getLine();

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.

Simplify testcase

Thanks for the patch! The testcase looks like it would bitrot very easily. I would recommend first manually reducing the IR as much as possible and then making a MIR testcase out of it to become independent from future ISEL changes.

test/DebugInfo/X86/debug-loc-0.ll
47 ↗(On Diff #181962)

I'm sure the function can be made shorter?

63 ↗(On Diff #181962)

We probably don't need most of these

74 ↗(On Diff #181962)

Looks like there is a lot of metadata that is not really relevant to the test. I think we should be able to strip everything that is not a DIFile/DIScope/DICompileUnit from the metadata and !llvm.* named mdnodes.

114 ↗(On Diff #181962)

We probably don't need more than one nonzero DILocation?

Reduce test case further and make it a MIR test.

aprantl added inline comments.Jan 16 2019, 11:25 AM
test/DebugInfo/MIR/X86/debug-loc-0.mir
23 ↗(On Diff #182105)

I don't think this is needed either, right? Line tables only should be sufficient,

48 ↗(On Diff #182105)

personally I would probably also remove all the paths

aprantl accepted this revision.Jan 16 2019, 11:26 AM

I think we can reduce the test even more, but otherwise this should be fine.

This revision is now accepted and ready to land.Jan 16 2019, 11:26 AM
This revision was automatically updated to reflect the committed changes.

Thanks! I'm not surprised I didn't get it completely right; it was such a maze of twisty passages all different.