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

[kamleshbhalui]

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.

[lebedev.ri]

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

3225

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

[davide]

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 ?

[kamleshbhalui]

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

[kamleshbhalui]

Formatted the diff.

[davide]

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

[davide]

I'll commit this for you later tonight.

[davide]

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.

[davide]

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.

[kamleshbhalui]

Oops, semicolon due to screwed-up formatting.

Removed the semicolon and updated other failing test cases.

[davide]

llvm/test/Transforms/InstCombine/debuginfo-dce.ll

36–37 ↗

(On Diff #227996)

Do you know why this changed?

[kamleshbhalui]

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]

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

3225

My comment wasn't addressed.

[djtodoro]

llvm/test/Transforms/InstCombine/pr43893.ll

27

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

[kamleshbhalui]

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]

removed unused declaration from testcase(pr43893.ll)

[kamleshbhalui]

llvm/test/Transforms/InstCombine/pr43893.ll

27

Yes deleted that.

[jmorse]

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.

[kamleshbhalui]

Cleaned up and added comments in testcase.

[jmorse]

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

[vsk]

@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.

[kamleshbhalui]

Incorporated @vsk comments.

[vsk]

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]

Go for it.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

 bool InstCombiner::run() {
   while (!Worklist.isEmpty()) {
     Instruction *I = Worklist.RemoveOne();
     if (I == nullptr) continue;  // skip null values.
 
     // Check to see if we can DCE the instruction.
     if (isInstructionTriviallyDead(I, &TLI)) {
       LLVM_DEBUG(dbgs() << "IC: DCE: " << *I << '\n');
+
+      SmallVector<DbgVariableIntrinsic *, 1> DbgUsers;
+      findDbgUsers(DbgUsers, I);
+      if (!DbgUsers.empty()) {
+        for (auto *DII : DbgUsers) {
+         Value *Undef = UndefValue::get(I->getType());
+         DII->setOperand(0, MetadataAsValue::get(DII->getContext(),
+                                            ValueAsMetadata::get(Undef)));
+  }
+      }
+
       eraseInstFromFunction(*I);
       ++NumDeadInst;
       MadeIRChange = true;
       continue;
     }
 
     if (!DebugCounter::shouldExecute(VisitCounter))
       continue;
 

llvm/test/Transforms/InstCombine/pr43893.ll

+; RUN: opt -instcombine -S -o - %s | FileCheck %s
+
+@a = common dso_local global i8 0, align 1, !dbg !0
+@b = common dso_local global i8 0, align 1, !dbg !6
+
+; Function Attrs: noinline nounwind uwtable
+define dso_local i32 @main() #0 !dbg !13 {
+entry:
+  %0 = load i8, i8* @a, align 1, !dbg !17
+  %dec = add i8 %0, -1, !dbg !17
+  store i8 %dec, i8* @a, align 1, !dbg !17
+;CHECK: call void @llvm.dbg.value(metadata i32 undef
+;CHECK: call void @llvm.dbg.value(metadata i32 -8
+;CHECK call void @llvm.dbg.value(metadata i32 undef
+  %conv = sext i8 %dec to i32, !dbg !17
+  call void @llvm.dbg.value(metadata i32 %conv, metadata !18, metadata !DIExpression()), !dbg !19
+  call void @llvm.dbg.value(metadata i32 -8, metadata !20, metadata !DIExpression()), !dbg !19
+  call void @llvm.dbg.value(metadata i32 %conv, metadata !20, metadata !DIExpression()), !dbg !19
+  store i8 0, i8* @b, align 1, !dbg !21
+  %cmp = icmp sgt i32 %conv, 0, !dbg !22
+  %conv1 = zext i1 %cmp to i32, !dbg !22
+  ret i32 0, !dbg !23
+}
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+attributes #0 = { noinline nounwind uwtable }
+attributes #1 = { nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!9, !10, !11}
+!llvm.ident = !{!12}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "a", scope: !2, file: !3, line: 1, type: !8, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 10.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5, nameTableKind: None)
+!3 = !DIFile(filename: "test", directory: "n")
+!4 = !{}
+!5 = !{!0, !6}
+!6 = !DIGlobalVariableExpression(var: !7, expr: !DIExpression())
+!7 = distinct !DIGlobalVariable(name: "b", scope: !2, file: !3, line: 1, type: !8, isLocal: false, isDefinition: true)
+!8 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+!9 = !{i32 2, !"Dwarf Version", i32 4}
+!10 = !{i32 2, !"Debug Info Version", i32 3}
+!11 = !{i32 1, !"wchar_size", i32 4}
+!12 = !{!"clang version 10.0.0"}
+!13 = distinct !DISubprogram(name: "main", scope: !3, file: !3, line: 2, type: !14, scopeLine: 2, spFlags: DISPFlagDefinition, unit: !2, retainedNodes: !4)
+!14 = !DISubroutineType(types: !15)
+!15 = !{!16}
+!16 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!17 = !DILocation(line: 4, column: 11, scope: !13)
+!18 = !DILocalVariable(name: "c", scope: !13, file: !3, line: 4, type: !16)
+!19 = !DILocation(line: 0, scope: !13)
+!20 = !DILocalVariable(name: "l_1240", scope: !13, file: !3, line: 6, type: !16)
+!21 = !DILocation(line: 10, column: 10, scope: !13)
+!22 = !DILocation(line: 10, column: 5, scope: !13)
+!23 = !DILocation(line: 12, column: 1, scope: !13)