When GVN removes a redundant load, it should not modify the debug location of the dominating load.

[andreadb]

Summary

Consider the following example:

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"

define i32 @foo(i32 %X, i32* %Y) !dbg !6 {
entry:
  %0 = load i32, i32* %Y, align 4, !dbg !8
  %cmp = icmp sgt i32 %X, -1, !dbg !9
  br i1 %cmp, label %if.then, label %if.end, !dbg !10

if.then:                                          ; preds = %entry
  %1 = load i32, i32* %Y, align 4, !dbg !11
  %add = add nsw i32 %0, %1, !dbg !12
  call void (...) @fubar() #2, !dbg !13
  br label %if.end, !dbg !14

if.end:                                           ; preds = %if.then, %entry
  %Result.0 = phi i32 [ %add, %if.then ], [ %0, %entry ]
  ret i32 %Result.0, !dbg !15
}

declare void @fubar(...)

Where:

!8 = !DILocation(line: 3, column: 16, scope: !6)
!9 = !DILocation(line: 5, column: 9, scope: !6)
!10 = !DILocation(line: 5, column: 7, scope: !6)
!11 = !DILocation(line: 6, column: 15, scope: !6)
!12 = !DILocation(line: 6, column: 12, scope: !6)
!13 = !DILocation(line: 7, column: 5, scope: !6)
!14 = !DILocation(line: 8, column: 3, scope: !6)
!15 = !DILocation(line: 10, column: 3, scope: !6)

GVN would remove the redundant load from basic block %if.then since it is trivially dominated by an equivalent load from the entry block.

The redundant load from %if.then is:

%1 = load i32, i32* %Y, align 4, !dbg !11

The dominating load from %entry is:

%0 = load i32, i32* %Y, align 4, !dbg !8

When removing the redundant load %1, GVN also updates the debug location of %0 with the debugloc of %1.

As a result, the load from %entry now incorrectly refers to "line: 6, column: 15" instead of "line: 3,
column: 16".

In the case of a fully redundant load LI dominated by an equivalent load V, GVN should always preserve the original debug location of V. Otherwise, we risk to introduce an incorrect stepping.
If V has debug info, then clearly it should not be modified. If V has a null debugloc, then it is still potentially incorrect to propagate LI's debugloc because LI may not post-dominate V (this is what happens in the example).

Please let me know if okay to commit.

Thanks,
Andrea

[aprantl]

This makes sense to me. Are there other cases where we should invoke the mergeDebugLoc API that we've been discussing in D26256?

lib/Transforms/Scalar/GVN.cpp

1704

This comment repeats what the code is doing but doesn't really say why this is important. Can you add some of the the rationale from the review here?

[andreadb]

In D27468#614929, @aprantl wrote:

This makes sense to me. Are there other cases where we should invoke the mergeDebugLoc API that we've been discussing in D26256?

Thanks Adrian.

So far, I didn't find any particular case in GVN which would benefit from that new API..

As a side note: that new API would be useful for r280995 (originally discussed at revision D24164).

lib/Transforms/Scalar/GVN.cpp

1704

Sure. I can add a comment similar to what I wrote in the description.
Something like this maybe:
"If I has debug info, then clearly it should not be modified. However, if I has a null debugloc, then it is still potentially incorrect to propagate LI's debugloc because LI may not post-dominate I".

lib/Transforms/Scalar/GVN.cpp

if (UnavailableBlocks.empty()) {
 
     // Perform PHI construction.
     Value *V = ConstructSSAForLoadSet(LI, ValuesPerBlock, *this);
     LI->replaceAllUsesWith(V);
 
     if (isa<PHINode>(V))
       V->takeName(LI);
     if (Instruction *I = dyn_cast<Instruction>(V))
-      if (LI->getDebugLoc())
+      // If we have a fully redundant load LI dominated by a single value I,
+      // then don't update the debug location of I.
+      if (LI->getDebugLoc() && ValuesPerBlock.size() != 1)
         I->setDebugLoc(LI->getDebugLoc());
     if (V->getType()->getScalarType()->isPointerTy())
       MD->invalidateCachedPointerInfo(V);
     markInstructionForDeletion(LI);
     ++NumGVNLoad;
     reportLoadElim(LI, V, ORE);
     return true;
   }

test/Transforms/GVN/dbg-redundant-load.ll

+; RUN: opt -gvn -S < %s | FileCheck %s
+
+; Check that the redundant load from %if.then is removed.
+; Also, check that the debug location associated to load %0 still refers to
+; line 3 and not line 6.
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+; CHECK: @test_redundant_load(
+; CHECK-LABEL: entry:
+; CHECK-NEXT: load i32, i32* %Y, align 4, !dbg ![[LOC:[0-9]+]]
+; CHECK-LABEL: if.then:
+; CHECK-NOT: load
+; CHECK-LABEL: if.end:
+; CHECK: ![[LOC]] = !DILocation(line: 3, scope: !{{.*}})
+
+define i32 @test_redundant_load(i32 %X, i32* %Y) !dbg !6 {
+entry:
+  %0 = load i32, i32* %Y, align 4, !dbg !8
+  %cmp = icmp sgt i32 %X, -1, !dbg !9
+  br i1 %cmp, label %if.then, label %if.end, !dbg !9
+
+if.then:                                          ; preds = %entry
+  %1 = load i32, i32* %Y, align 4, !dbg !10
+  %add = add nsw i32 %0, %1, !dbg !10
+  call void @foo(), !dbg !11
+  br label %if.end, !dbg !12
+
+if.end:                                           ; preds = %if.then, %entry
+  %Result.0 = phi i32 [ %add, %if.then ], [ %0, %entry ]
+  ret i32 %Result.0, !dbg !13
+}
+
+declare void @foo()
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, isOptimized: false, runtimeVersion: 0, emissionKind: LineTablesOnly, enums: !2)
+!1 = !DIFile(filename: "test.cpp", directory: "")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"PIC Level", i32 2}
+!6 = distinct !DISubprogram(name: "test_redundant_load", scope: !1, file: !1, line: 2, type: !7, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!7 = !DISubroutineType(types: !2)
+!8 = !DILocation(line: 3, scope: !6)
+!9 = !DILocation(line: 5, scope: !6)
+!10 = !DILocation(line: 6, scope: !6)
+!11 = !DILocation(line: 7, scope: !6)
+!12 = !DILocation(line: 8, scope: !6)
+!13 = !DILocation(line: 10, scope: !6)