This is an archive of the discontinued LLVM Phabricator instance.

[deadargelim] Attach dbg info to the insert/extractvalue instructions
ClosedPublic

Authored by djtodoro on Jun 16 2020, 7:19 AM.

Details

Summary

The bug was found by the LLVM DI Checker (pushed as -debugify=original mode) and this was used in the RFC [0] for the utility.

Attach DbgLoc on insertvalue/extractvalue instructions created by DeadArgumentElimination.
This fixes the PR46350.

[0] https://groups.google.com/forum/#!msg/llvm-dev/QOyF-38YPlE/G213uiuwCAAJ

Diff Detail

Event Timeline

djtodoro created this revision.Jun 16 2020, 7:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2020, 7:19 AM
vsk added a subscriber: vsk.Jun 16 2020, 12:19 PM

Hi @djtodoro, this popped up in my mail filter so I thought I'd drop a line :). It sounds like there's a substantial amount of overlap with the debugify stuff and the tool here (https://github.com/djolertrk/llvm-di-checker). Do we need both?

For the issue found here for example, one might do: ./bin/llvm-lit -Dopt="opt -debugify-each" test/Transforms/DeadArgElim -av 2>&1 | less, and this prints:

PASS: LLVM :: Transforms/DeadArgElim/multdeadretval.ll (28 of 34)
Script:
--
: 'RUN: at line 5';   opt -debugify-each < /Users/vsk/src/llvm-project-master/llvm/test/Transforms/DeadArgElim/multdeadretval.ll -deadargelim -instcombine -dce -S | /Users/vsk/src/builds/llvm-project-master-RA/bin/not grep i16
--
Exit Code: 0

Command Output (stderr):
--
ERROR: Instruction with empty DebugLoc in function test --  %oldret = extractvalue { i16, i32 } %B, 1
ERROR: Instruction with empty DebugLoc in function test2 --  %oldret = extractvalue { i32, i16 } %B, 0
ERROR: Instruction with empty DebugLoc in function test3 --  %oldret = insertvalue { i16, i32 } undef, i32 %ret, 1
ERROR: Instruction with empty DebugLoc in function test5 --  %oldret = extractvalue { i32, i32, i16 } %C, 0
...

I think there are advantages to looking at real code outside of the llvm unit/regression tests, just wondering whether we need to redo all the DI checking / stats collection work.

In D81939#2096463, @vsk wrote:

Hi @djtodoro, this popped up in my mail filter so I thought I'd drop a line :). It sounds like there's a substantial amount of overlap with the debugify stuff and the tool here (https://github.com/djolertrk/llvm-di-checker). Do we need both?

For the issue found here for example, one might do: ./bin/llvm-lit -Dopt="opt -debugify-each" test/Transforms/DeadArgElim -av 2>&1 | less, and this prints:

PASS: LLVM :: Transforms/DeadArgElim/multdeadretval.ll (28 of 34)
Script:
--
: 'RUN: at line 5';   opt -debugify-each < /Users/vsk/src/llvm-project-master/llvm/test/Transforms/DeadArgElim/multdeadretval.ll -deadargelim -instcombine -dce -S | /Users/vsk/src/builds/llvm-project-master-RA/bin/not grep i16
--
Exit Code: 0

Command Output (stderr):
--
ERROR: Instruction with empty DebugLoc in function test --  %oldret = extractvalue { i16, i32 } %B, 1
ERROR: Instruction with empty DebugLoc in function test2 --  %oldret = extractvalue { i32, i16 } %B, 0
ERROR: Instruction with empty DebugLoc in function test3 --  %oldret = insertvalue { i16, i32 } undef, i32 %ret, 1
ERROR: Instruction with empty DebugLoc in function test5 --  %oldret = extractvalue { i32, i32, i16 } %C, 0
...

I think there are advantages to looking at real code outside of the llvm unit/regression tests, just wondering whether we need to redo all the DI checking / stats collection work.

Hi @vsk, thanks for the comment! :)

The debugify tool is very useful for the regression testing/Pass verification; and I use it that way and I think we can all use it in the combination with this. The idea here was to introduce a tool (let me say a sibling of the debugify) that will deal with real debug info metadata. I know that debugify already deals with the DILocations, but I thought to introduce support for all kind of metadata (including DIGlobalVariables, DILexicalBlocks, etc.; which is hard to maintain as synthetic debug info), and support DILocations along the way as well. In addition, I think that extending this to MIR level is also doable/may be straight forward.

We all use the compiler on real projects, and users report issues regarding debug info saying "My variable 'a' is optimized out, but it should not be" or "I cannot attach breakpoint to function 'f' (or instruction 'i')", so I found this tool very useful in these situations, since it points (or it will point when improved; this is initial stage) to real spot of the bug that caused the issue for the concrete entity. Also, there are companies with downstream passes and frontend features that may not be direct cause of the bug, but the bug can occur somewhere else in the pipeline as a consequence. So, I think (by using this) we can come up with variety of test cases that may not be present in the existing tests.

I'll send an RFC and please comment on that! Thanks again!

djtodoro updated this revision to Diff 274464.Jun 30 2020, 7:21 AM
djtodoro retitled this revision from [NOT FOR COMMIT] [deadargelim] Attach dbg info to the insert/extractvalue instructions to [deadargelim] Attach dbg info to the insert/extractvalue instructions.
djtodoro edited the summary of this revision. (Show Details)
djtodoro added reviewers: vsk, aprantl, probinson, dblaikie.
djtodoro set the repository for this revision to rG LLVM Github Monorepo.
  • Cover all the instructions
  • Cover all the cases with the test
  • Remove [NOT FOR COMMIT] part
aprantl added inline comments.Jun 30 2020, 2:14 PM
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
973

@vsk: would it be better style to ad-hoc create an IRBuilder with the correct debug location here?

llvm/test/Transforms/DeadArgElim/multdeadretval.ll
11

I think it would be much better to have a targeted check here that checks the actual instructions. Debugify is great for finding bugs, but I wouldn't recommend it for writing testcases. In this form, nothing guarantees that the code path that triggers the bugfix is still exercised by the test if the pass is modified and the test quickly looses its usefulness.

djtodoro updated this revision to Diff 274711.Jul 1 2020, 1:17 AM
  • Check the actual instructions instead of final debugify result in the test provided
djtodoro marked 2 inline comments as done.Jul 1 2020, 1:18 AM
djtodoro added inline comments.
llvm/test/Transforms/DeadArgElim/multdeadretval.ll
11

I've just addressed this.

aprantl added inline comments.Jul 1 2020, 9:32 AM
llvm/test/Transforms/DeadArgElim/multdeadretval.ll
12

Thanks!

Should we also check that the !dbg attachment is the same as some other instruction? Or are we really fine with just *any* !dbg location?

djtodoro marked 3 inline comments as done.Jul 2 2020, 2:08 AM
djtodoro added inline comments.
llvm/test/Transforms/DeadArgElim/multdeadretval.ll
12

It makes sense to me to test the actual locations. I've addressed that as well. Thanks you @aprantl!

djtodoro updated this revision to Diff 275022.Jul 2 2020, 2:09 AM
djtodoro marked an inline comment as done.
  • Check actual locations in the test
vsk added inline comments.Jul 5 2020, 5:37 PM
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
973

Yes, that seems to be the common idiom. I recommend using IRBuilder<NoFolder> to avoid spurious test changes.

llvm/test/Transforms/DeadArgElim/multdeadretval.ll
7

I recommend copying this test, modifying it to include debug info, and dropping the -enable-debugify=synthetic part. This bugfix doesn't need to depend on the debugify original mode patchset. Also, the hardcoded checks for DILocation line numbers will make this test hard to modify, so if we want to check specific synthetic line numbers I think we'd be better served by a dedicated test.

djtodoro marked an inline comment as done.Jul 6 2020, 6:22 AM
djtodoro added inline comments.
llvm/test/Transforms/DeadArgElim/multdeadretval.ll
7

It makes sense to me! Thanks!

djtodoro marked an inline comment as done.Jul 6 2020, 6:24 AM
djtodoro added inline comments.
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
973

Oh.. The IRBuilder<> will generate a debug loc by default (via insert() method).

djtodoro updated this revision to Diff 275695.Jul 6 2020, 6:27 AM
  • Use IRBuilder<> since it generates dbg loc by default
  • Create new test

lgtm from my point of view now.

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
981–982

Nice!

llvm/test/DebugInfo/X86/dbgloc-insert-extract-val-instrs.ll
5 ↗(On Diff #275695)

Why use a custom prefix if there is only one FileCheck invocation?

219 ↗(On Diff #275695)

We might as well delete all the column: 1 fields, assuming that 0 is the default.

vsk added a comment.Jul 9 2020, 10:18 AM

Are test{1,2,3,4,5,6} and main all necessary to exercise the changes in this patch? On the surface, it looks like there are two primary changes -- one that affects the case when deadargelim changes the function return type, and another that affects the case where deadargelim modifies a function that returns an array/struct. Can the test be pared down to just cover those two cases?

llvm/test/DebugInfo/X86/dbgloc-insert-extract-val-instrs.ll
4 ↗(On Diff #275695)

It doesn't look like the -check-debugify output is important, so it shouldn't be necessary to run the pass.

Also, since the dbg.values are also not important, please run -debugify with -debugify-level=locations to omit those intrinsics.

7 ↗(On Diff #275695)

Please restructure these checks so they have a clear correspondence to a test function. The typical way to write this is:

; CHECK-LABEL: some_test1
; CHECK: ...
define void @some_test1

; CHECK-LABEL: some_test2
; CHECK: ...
define void @some_test2

etc.

djtodoro marked 3 inline comments as done.Jul 10 2020, 2:32 AM

@aprantl @vsk Thanks a lot for the feedback!

llvm/test/DebugInfo/X86/dbgloc-insert-extract-val-instrs.ll
4 ↗(On Diff #275695)

I see..thanks :)

5 ↗(On Diff #275695)

Oh, sure..

7 ↗(On Diff #275695)

I was a bit lazy, sure, thanks!

djtodoro updated this revision to Diff 276966.Jul 10 2020, 2:38 AM
  • Addressing comments
  • Reduce the test
djtodoro updated this revision to Diff 276969.Jul 10 2020, 2:46 AM
  • [test] remove unused debugify metadata
vsk accepted this revision.Jul 13 2020, 1:49 PM

Thanks, lgtm!

This revision is now accepted and ready to land.Jul 13 2020, 1:49 PM
This revision was automatically updated to reflect the committed changes.