This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by andreadb on Dec 6 2016, 9:46 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

andreadb updated this revision to Diff 80436.Dec 6 2016, 9:46 AM
andreadb retitled this revision from to When GVN removes a redundant load, it should not modify the debug location of the dominating load..
andreadb updated this object.
andreadb added a subscriber: llvm-commits.
gbedwell removed a subscriber: gbedwell.
gbedwell added a subscriber: gbedwell.
aprantl edited edge metadata.Dec 6 2016, 12:38 PM

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

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?

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

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

aprantl accepted this revision.Dec 6 2016, 1:11 PM
aprantl edited edge metadata.
This revision is now accepted and ready to land.Dec 6 2016, 1:11 PM
This revision was automatically updated to reflect the committed changes.