Page MenuHomePhabricator

Wrong debug info generated at -O2 (-O0 is correct)
ClosedPublic

Authored by kamleshbhalui on Nov 4 2019, 9:49 AM.

Details

Summary

Instcombiner pass was erasing trivially dead instruction without updating dependent llvm.dbg.value.
which was not showing programmer current state of variables while debugging.
As a part of this fix I did following,
Iterate throught all the users (llvm.dbg) of a instruction which is trivially dead and set each if them undef, Before deleting the instruction.
Now user will see optimized out, when try to print those variables.
This fixes
https://bugs.llvm.org/show_bug.cgi?id=43893

This is my first fix to llvm.

Diff Detail

Repository
rL LLVM

Event Timeline

kamleshbhalui created this revision.Nov 4 2019, 9:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2019, 9:49 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
lebedev.ri added inline comments.
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3225

What about other users of eraseInstFromFunction()?
Should this go into that function?

davide requested changes to this revision.Nov 4 2019, 10:17 AM
davide added a subscriber: davide.
davide added inline comments.
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3215–3223

I think there should be an utility to do this. Did you check what other callers of isInstructionTriviallyDead do? e.g. EarlyCSE ?

This revision now requires changes to proceed.Nov 4 2019, 10:17 AM

Used the existing utility, which can be seen in other places too.

Formatted the diff.

davide accepted this revision.Nov 5 2019, 3:37 PM
davide added a subscriber: vsk.

LGTM. @aprantl / @vsk ? Do you need somebody to commit this for you?

This revision is now accepted and ready to land.Nov 5 2019, 3:37 PM
davide added a reviewer: vsk.Nov 5 2019, 3:37 PM
davide added a comment.Nov 5 2019, 3:58 PM

I'll commit this for you later tonight.

davide requested changes to this revision.Nov 5 2019, 5:54 PM

uh, how can this possibly work?

[1/47] Building CXX object lib/Transforms/InstCombine/CMakeFiles/LLVMInstCombine.dir/InstructionCombining.cpp.o
/Users/dcci/work/llvm-mono/llvm-project/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3237:33: warning: if statement has empty body [-Wempty-body]
      if (!salvageDebugInfo(*I));
                                ^
/Users/dcci/work/llvm-mono/llvm-project/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3237:33: note: put the semicolon on a separate line to silence this warning
1 warning generated.
This revision now requires changes to proceed.Nov 5 2019, 5:54 PM
davide added a comment.Nov 5 2019, 6:02 PM

If I fix your patch by removing the semicolon, I see three tests failing.

Testing Time: 412.65s
********************
Failing Tests (3):
    LLVM :: Transforms/InstCombine/cast-mul-select.ll
    LLVM :: Transforms/InstCombine/debuginfo-dce.ll
    LLVM :: Transforms/InstCombine/stacksave-debuginfo.ll

  Expected Passes    : 32961
  Expected Failures  : 151
  Unsupported Tests  : 1107
  Unexpected Failures: 3

Can you please look at these [and make sure you rerun ninja check-llvm if you modify the patch?]. Thanks.

Oops, semicolon due to screwed-up formatting.

Removed the semicolon and updated other failing test cases.

davide added inline comments.Nov 5 2019, 7:58 PM
llvm/test/Transforms/InstCombine/debuginfo-dce.ll
36–37 ↗(On Diff #227996)

Do you know why this changed?

kamleshbhalui added inline comments.Nov 5 2019, 9:40 PM
llvm/test/Transforms/InstCombine/debuginfo-dce.ll
36–37 ↗(On Diff #227996)

Before the fix instcombiner was deleting all the users (even dbg.value ) of a trivially dead instruction.
But now we mark dbg.value as undef instead of deleting it .
and that's why modified the check like this.

lebedev.ri requested changes to this revision.Nov 5 2019, 11:02 PM
lebedev.ri added inline comments.
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3225

My comment wasn't addressed.

This revision now requires changes to proceed.Nov 5 2019, 11:02 PM
djtodoro added inline comments.
llvm/test/Transforms/InstCombine/pr43893.ll
27

This is unused. We can delete this from the test.

kamleshbhalui marked an inline comment as done.Nov 6 2019, 12:13 AM
kamleshbhalui added inline comments.
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3225

I have seen this is in EarlyCSE.
And yes, It should go into several other places too, But need to come up with testcases.

kamleshbhalui marked an inline comment as done.

removed unused declaration from testcase(pr43893.ll)

kamleshbhalui marked an inline comment as done.Nov 6 2019, 2:50 AM
kamleshbhalui added inline comments.
llvm/test/Transforms/InstCombine/pr43893.ll
27

Yes deleted that.

jmorse added a comment.Nov 6 2019, 4:13 AM

A few nits inline, otherwise great!

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3225

replaceDbgUsesWithUndef should quite possibly go into the intstruction-deleting methods, D69787 even puts salvageDebugInfo in those methods too. It's not clear to me why this hasn't always been the case (performance)? IMO, that would be another patch though.

llvm/test/Transforms/InstCombine/debuginfo-dce.ll
36–37 ↗(On Diff #227996)

Best to give the checked-for dbg.value an operand, so "dbg.value(metadata %struct.entry *undef" or similar. Otherwise this isn't fully checking for the correct behaviour here.

llvm/test/Transforms/InstCombine/pr43893.ll
2

It'd be nice to have a summary comment at the top of the test, explaining what it's testing for.

33

We usually delete all attributes from tests, as they're an un-necessary maintenance burden.

Cleaned up and added comments in testcase.

jmorse accepted this revision.Nov 6 2019, 6:39 AM

LGTM, although it's worth leaving this 24h in case other reviewers have an opinion.

vsk added a comment.Nov 6 2019, 11:41 AM

@kamleshbhalui thanks for working on this, and welcome to llvm :)

eraseInstFromFunction already calls salvageDebufInfo: why not have it call replaceDbgUsesWithUndef as well, or better, define a salvageDebufInfoOrMarkUndef helper? I'd expect that to be reasonable to tackle in this patch.

llvm/test/Transforms/InstCombine/stacksave-debuginfo.ll
11 ↗(On Diff #228044)

Please do not hardcode metadata numbers in check lines. If the variable referenced by a metadata node is obvious from context, it's better not to match the number at all.

Incorporated @vsk comments.

vsk accepted this revision.Nov 6 2019, 5:44 PM

Looks great, thanks. @lebedev.ri, @davide -- it looks like your comments are addressed. All tests pass locally for me. I'll wait a day to be safe, then land this on kamlesh's behalf if no one beats me to it.

davide added a comment.Nov 6 2019, 5:57 PM

Go for it.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 7 2019, 11:25 AM
This revision was automatically updated to reflect the committed changes.