Page MenuHomePhabricator

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

Authored by djtodoro on Tue, Jun 16, 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.Tue, Jun 16, 7:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jun 16, 7:19 AM
vsk added a subscriber: vsk.Tue, Jun 16, 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.Tue, Jun 30, 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.Tue, Jun 30, 2:14 PM
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
975–976

@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 ↗(On Diff #274464)

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.Wed, Jul 1, 1:17 AM
  • Check the actual instructions instead of final debugify result in the test provided
djtodoro marked 2 inline comments as done.Wed, Jul 1, 1:18 AM
djtodoro added inline comments.
llvm/test/Transforms/DeadArgElim/multdeadretval.ll
11 ↗(On Diff #274464)

I've just addressed this.

aprantl added inline comments.Wed, Jul 1, 9:32 AM
llvm/test/Transforms/DeadArgElim/multdeadretval.ll
12 ↗(On Diff #274711)

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.Thu, Jul 2, 2:08 AM
djtodoro added inline comments.
llvm/test/Transforms/DeadArgElim/multdeadretval.ll
12 ↗(On Diff #274711)

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.Thu, Jul 2, 2:09 AM
djtodoro marked an inline comment as done.
  • Check actual locations in the test
vsk added inline comments.Sun, Jul 5, 5:37 PM
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
975–976

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 ↗(On Diff #275022)

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.Mon, Jul 6, 6:22 AM
djtodoro added inline comments.
llvm/test/Transforms/DeadArgElim/multdeadretval.ll
7 ↗(On Diff #275022)

It makes sense to me! Thanks!

djtodoro marked an inline comment as done.Mon, Jul 6, 6:24 AM
djtodoro added inline comments.
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
975–976

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

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