Page MenuHomePhabricator

[Docs] Testing Debug Info Preservation in Optimizations

Authored by gramanas on Jul 7 2018, 10:36 AM.



Add section in SourceLevelDebugging.rst with information
about debugify and how to use it in regression tests.

Diff Detail


Event Timeline

gramanas created this revision.Jul 7 2018, 10:36 AM
gramanas updated this revision to Diff 154583.Jul 9 2018, 5:42 AM
  • Code blocks had some whitespace
  • Check that arc works with new dev envrionment
vsk added inline comments.Jul 9 2018, 11:49 AM
1482 ↗(On Diff #154583)

"non null" is implied here, please omit it.

1485 ↗(On Diff #154583)

Could you add a section explaining what kind of debug information is attached? Specifically: instructions are assigned sequentially-increasing line locations, and are immediately used by debug value intrinsics when possible. It would help if the first example is a comparison of a short function before/after -debugify is applied.

1486 ↗(On Diff #154583)

This is a good example, but it should come after the one which shows a before/after with -debugify. Please explain what the error message means and how (at a high level) to fix it. You also only really need one ERROR line here, please omit the rest.

1527 ↗(On Diff #154583)

Please show an example of using -debugify to test a backend, e.g:
$ opt -debugify < sample.ll | llc -o -

1540 ↗(On Diff #154583)

I think it'd be better to use a real, concrete example here. Please find one with stricter CHECK lines from the perspective of the actual optimization (e.g check the operands of the mul).

1551 ↗(On Diff #154583)

Please justify the '2' here to readers (btw why isn't it 1?). It's worth adding a note that checking for a DILocation in this way is brittle. If someone updating the test inserts an instruction in the original IR before the second instruction, the test will break. Discuss the tradeoff between having a brittle test, and a test that is stringent enough to check for correct debug location preservation.

1554 ↗(On Diff #154583)

It's not quite enough to say that the mul corresponds to line 2. The test should be checking that the mul looks correct after -pass-to-test runs, and it should explain why line 2 is the correct line for the mul to be on.

gramanas updated this revision to Diff 154851.Jul 10 2018, 11:27 AM
gramanas marked 5 inline comments as done.

Address inline comments.

This comment was removed by gramanas.
vsk added a comment.Jul 10 2018, 11:56 AM

This is looking really nice already! Some more suggestions inline --

1499 ↗(On Diff #154851)

It looks like this is under-indented by 3 spaces?

1514 ↗(On Diff #154851)

Ditto, it looks like the line containing the 'alloca' is over-indented, and some of the other lines are under-indented by 3 spaces.

1546 ↗(On Diff #154851)

'Instructions', 'Debug', and 'Location' don't need to be capitalized here.

1547 ↗(On Diff #154851)

Maybe better worded as: "to an instruction having a type that's incompatible with the source variable it describes"

1553 ↗(On Diff #154851)

The 'Fixing errors' section should be at a higher level. Please describe which APIs you'd use to fix each issue and why. You don't need to include code examples of how these APIs are used: assume that a developer reading this can find the relevant documentation and examples. Also, assume that readers will be able to locate problematic areas in the code.

Missing debug location => Instruction::setDebugLoc, or possibly IRBuilder::setCurrentDebugLocation when using a Builder and the new location should be reused
Debug value with incompatible type => llvm::replaceAllDbgUsesWith (explain why the incompatible type error can occur after a RAUW, and how this utility prevents it)
Missing debug value => llvm::salvageDebugInfo (when no replacement exists), or llvm::replaceAllDbgUsesWith (when a replacement exists)

1585 ↗(On Diff #154851)

Please always capitalize sentences and end them with periods.

gramanas updated this revision to Diff 155016.Jul 11 2018, 8:51 AM
gramanas marked 6 inline comments as done.
  • Address inline comments.

In the debugify utility section I can't get the llvm code blocks
to print correctly. I can't spot any difference with the last one
in debugify in regression tests yet in the compiled html this first
two show like plain monospace while the last is colored correctly.

vsk added a comment.Jul 11 2018, 9:38 AM

This looks really close, thanks! Phabricator is rendering all of the code blocks correctly -- there doesn't seem to be a formatting issue on this end.

1468 ↗(On Diff #155016)

I don't know how I missed this earlier, but I think the title should be a little more specific. This section is more about 'testing optimizations with debug info', than it is about actually debugging optimized code.

1563 ↗(On Diff #155016)

'my moving instructions' -> 'salvaging or deleting invalid debug values'

1634 ↗(On Diff #155016)

I still think we should add a caveat here. In order to write this type of regression test in a way that's not brittle, it's important to avoid hardcoding line/variable numbers in check lines (please explain why). In cases where that can't be avoided (say, if a test wouldn't be precise enough), it's a good practice to move the test into its own file.

gramanas updated this revision to Diff 155398.Jul 13 2018, 8:27 AM
gramanas marked 3 inline comments as done.
  • change section title to reflect contents
  • add note for robust tests
gramanas updated this revision to Diff 155402.Jul 13 2018, 8:40 AM
  • minor styling changes
gramanas retitled this revision from [WIP][Docs] Debugging optimized code with debugify to [Docs] Debugging optimized code with debugify.Jul 13 2018, 8:41 AM
vsk accepted this revision.Jul 13 2018, 10:03 AM
vsk added a subscriber: debug-info.

Thanks! LGTM, this seems like it's in great shape.

Could you wait a few days before committing this? Others may have further corrections/improvements to suggest.

This revision is now accepted and ready to land.Jul 13 2018, 10:03 AM
vsk added a comment.Jul 16 2018, 4:23 PM

Please change the patch title to match the title of the newly-added section. I have a few more minor suggestions inline, but overall this is ready to land.

1591 ↗(On Diff #155402)

"append -check-debugify" -> "append -check-debugify -strip"

1604 ↗(On Diff #155402)

"first pass that applies synthetic DI" -> "-debugify pass"

1608 ↗(On Diff #155402)

Here's a way to rephrase the last sentence for extra clarity: 'Changes to this pass are not allowed to break existing tests.'

1617 ↗(On Diff #155402)

"-inst-combine" -> "-instcombine"

gramanas retitled this revision from [Docs] Debugging optimized code with debugify to [Docs] Testing Debug Info Preservation in Optimizations.Jul 17 2018, 9:21 AM
gramanas updated this revision to Diff 156264.Jul 19 2018, 7:08 AM
gramanas marked 4 inline comments as done.

Adressing latest comments

This revision was automatically updated to reflect the committed changes.