[Docs] Testing Debug Info Preservation in Optimizations

[gramanas]

Summary

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

[gramanas]

• Code blocks had some whitespace
• Check that arc works with new dev envrionment

[vsk]

docs/SourceLevelDebugging.rst

1482

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

1485

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

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

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

1540

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

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

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]

Address inline comments.

[gramanas]

This comment was removed by gramanas.

[vsk]

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

docs/SourceLevelDebugging.rst

1499

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

1514

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

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

1547

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

1553

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

Please always capitalize sentences and end them with periods.

[gramanas]

• 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]

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.

docs/SourceLevelDebugging.rst

1468

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

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

1634

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]

• change section title to reflect contents
• add note for robust tests

[gramanas]

• minor styling changes

[vsk]

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.

[vsk]

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.

docs/SourceLevelDebugging.rst

1591

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

1604

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

1608

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

1617

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

[gramanas]

Adressing latest comments

docs/SourceLevelDebugging.rst

 debug info formats such as STABS.
 
 It would also be reasonable to use debug information to feed profiling tools
 for analysis of generated code, or, tools for reconstructing the original
 source from generated code.
 
 .. _intro_debugopt:
 
-Debugging optimized code
-------------------------
+Debug information and optimizations
+-----------------------------------
 
 An extremely high priority of LLVM debugging information is to make it interact
 well with optimizations and analysis.  In particular, the LLVM debug
 information provides the following guarantees:
 
 * LLVM debug information **always provides information to accurately read
   the source-level state of the program**, regardless of which LLVM
   optimizations have been run, and without any modification to the
* Generate and dump CodeView from LLVM IR metadata::
     $ llvm-readobj -codeview foo.obj > foo.txt
 
   Use this pattern in lit test cases and FileCheck the output of llvm-readobj
 
 Improving LLVM's CodeView support is a process of finding interesting type
 records, constructing a C++ test case that makes MSVC emit those records,
 dumping the records, understanding them, and then generating equivalent records
 in LLVM's backend.
+
+Testing Debug Info Preservation in Optimizations
+================================================
+
+The following paragraphs are an introduction to the debugify utility
+and examples of how to use it in regression tests to check debug info
+preservation after optimizations.
+
+The ``debugify`` utility
+------------------------
+
+The ``debugify`` synthetic debug info testing utility consists of two
+main parts. The ``debugify`` pass and the ``check-debugify`` one. They are
+meant to be used with ``opt`` for development purposes.
+
+The first applies synthetic debug information to every instruction of the module,
+while the latter checks that this DI is still available after an optimization
+has occurred, reporting any errors/warnings while doing so.
+
+The instructions are assigned sequentially increasing line locations,
+and are immediately used by debug value intrinsics when possible.
+
+For example, here is a module before:
+
+.. code-block:: llvm
+
+   define dso_local void @f(i32* %x) {
+   entry:
+     %x.addr = alloca i32*, align 8
+     store i32* %x, i32** %x.addr, align 8
+     %0 = load i32*, i32** %x.addr, align 8
+     store i32 10, i32* %0, align 4
+     ret void
+   }
+
+and after running ``opt -debugify``  on it we get:
+
+.. code-block:: llvm
+
+   define dso_local void @f(i32* %x) !dbg !6 {
+   entry:
+     %x.addr = alloca i32*, align 8, !dbg !12
+     call void @llvm.dbg.value(metadata i32** %x.addr, metadata !9, metadata !DIExpression()), !dbg !12
+     store i32* %x, i32** %x.addr, align 8, !dbg !13
+     %0 = load i32*, i32** %x.addr, align 8, !dbg !14
+     call void @llvm.dbg.value(metadata i32* %0, metadata !11, metadata !DIExpression()), !dbg !14
+     store i32 10, i32* %0, align 4, !dbg !15
+     ret void, !dbg !16
+   }
+
+   !llvm.dbg.cu = !{!0}
+   !llvm.debugify = !{!3, !4}
+   !llvm.module.flags = !{!5}
+
+   !0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+   !1 = !DIFile(filename: "debugify-sample.ll", directory: "/")
+   !2 = !{}
+   !3 = !{i32 5}
+   !4 = !{i32 2}
+   !5 = !{i32 2, !"Debug Info Version", i32 3}
+   !6 = distinct !DISubprogram(name: "f", linkageName: "f", scope: null, file: !1, line: 1, type: !7, isLocal: false, isDefinition: true, scopeLine: 1, isOptimized: true, unit: !0, retainedNodes: !8)
+   !7 = !DISubroutineType(types: !2)
+   !8 = !{!9, !11}
+   !9 = !DILocalVariable(name: "1", scope: !6, file: !1, line: 1, type: !10)
+   !10 = !DIBasicType(name: "ty64", size: 64, encoding: DW_ATE_unsigned)
+   !11 = !DILocalVariable(name: "2", scope: !6, file: !1, line: 3, type: !10)
+   !12 = !DILocation(line: 1, column: 1, scope: !6)
+   !13 = !DILocation(line: 2, column: 1, scope: !6)
+   !14 = !DILocation(line: 3, column: 1, scope: !6)
+   !15 = !DILocation(line: 4, column: 1, scope: !6)
+   !16 = !DILocation(line: 5, column: 1, scope: !6)
+
+The following is an example of the -check-debugify output:
+
+.. code-block:: none
+
+   $ opt -enable-debugify -loop-vectorize llvm/test/Transforms/LoopVectorize/i8-induction.ll -disable-output
+   ERROR: Instruction with empty DebugLoc in function f --  %index = phi i32 [ 0, %vector.ph ], [ %index.next, %vector.body ]
+
+Errors/warnings can range from instructions with empty debug location to an
+instruction having a type that's incompatible with the source variable it describes,
+all the way to missing lines and missing debug value intrinsics.
+
+Fixing errors
+^^^^^^^^^^^^^
+
+Each of the errors above has a relevant API available to fix it.
+
+* In the case of missing debug location, ``Instruction::setDebugLoc`` or possibly
+  ``IRBuilder::setCurrentDebugLocation`` when using a Builder and the new location
+  should be reused.
+
+* When a debug value has incompatible type ``llvm::replaceAllDbgUsesWith`` can be used.
+  After a RAUW call an incompatible type error can occur because RAUW does not handle
+  widening and narrowing of variables while ``llvm::replaceAllDbgUsesWith`` does. It is
+  also capable of changing the DWARF expression used by the debugger to describe the variable.
+  It also prevents use-before-def by salvaging or deleting invalid debug values.
+
+* When a debug value is missing ``llvm::salvageDebugInfo`` can be used when no replacement
+  exists, or ``llvm::replaceAllDbgUsesWith`` when a replacement exists.
+
+Using ``debugify``
+------------------
+
+In order for ``check-debugify`` to work, the DI must be coming from
+``debugify``. Thus, modules with existing DI will be skipped.
+
+The most straightforward way to use ``debugify`` is as follows::
+
+  $ opt -debugify -pass-to-test -check-debugify sample.ll
+
+This will inject synthetic DI to ``sample.ll`` run the ``pass-to-test``
+and then check for missing DI.
+
+Some other ways to run debugify are avaliable:
+
+.. code-block:: bash
+
+   # Same as the above example.
+   $ opt -enable-debugify -pass-to-test sample.ll
+
+   # Suppresses verbose debugify output.
+   $ opt -enable-debugify -debugify-quiet -pass-to-test sample.ll
+
+   # Prepend -debugify before and append -check-debugify after
+   # each pass on the pipeline (similar to -verify-each).
+   $ opt -debugify-each -O2 sample.ll
+
+``debugify`` can also be used to test a backend, e.g:
+
+.. code-block:: bash
+
+   $ opt -debugify < sample.ll | llc -o -
+
+``debugify`` in regression tests
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The first pass that applies synthetic DI is especially helpful when it comes
+to testing that a given pass preserves DI while transforming the module.
+For this to work, the ``-debugify`` output must be stable enough to use in
+regression tests. There might be occasional bug-fixes but they are not allowed
+to break existing tests or alter their meaning.
+
+It allows us to test for DI loss in the same tests we check that the
+transformation is actually doing what it should.
+
+Here is an example from ``test/Transforms/InstCombine/cast-mul-select.ll``:
+
+.. code-block:: llvm
+
+   ; RUN: opt < %s -debugify -inst-combine -S | FileCheck %s --check-prefix=DEBUGINFO
+
+   define i32 @mul(i32 %x, i32 %y) {
+   ; DBGINFO-LABEL: @mul(
+   ; DBGINFO-NEXT:    [[C:%.*]] = mul i32 {{.*}}
+   ; DBGINFO-NEXT:    call void @llvm.dbg.value(metadata i32 [[C]]
+   ; DBGINFO-NEXT:    [[D:%.*]] = and i32 {{.*}}
+   ; DBGINFO-NEXT:    call void @llvm.dbg.value(metadata i32 [[D]]
+
+     %A = trunc i32 %x to i8
+     %B = trunc i32 %y to i8
+     %C = mul i8 %A, %B
+     %D = zext i8 %C to i32
+     ret i32 %D
+   }
+
+Here we test that the two ``dbg.value`` instrinsics are preserved and
+are correctly pointing to the ``[[C]]`` and ``[[D]]`` variables.
+
+.. note::
+
+   Note, that when writing this kind of regression tests, it is important
+   to make them as robust as possible. That's why we should try to avoid
+   hardcoding line/variable numbers in check lines. If for example you test
+   for a ``DILocation`` to have a specific line number, and someone later adds
+   an instruction before the one we check the test will fail. In the cases this
+   can't be avoided (say, if a test wouldn't be precise enough), moving the
+   test to it's own file is preferred.