[GVNHoist] fix a bug where GVNHoist preserves the debug location when it should be dropped

[yuanboli233]

Summary

It is a proposed patch for the bug presented here: https://bugs.llvm.org/show_bug.cgi?id=49099

In LLVM debug information update guide, "Hoisting identical instructions which appear in several successor blocks into a predecessor block. In this case, there is no single merged instruction. The rule for dropping locations applies."

Here in the code, the hoisted instruction keeps the previous debug location. If there are multiple of the instructions hoisted, then a conservative approach is possibly dropping the debug location.

I also provide a test case here.

If the patch makes sense, can you help commit it? Thanks.

[yuanboli233]

updated with git-clang-format

[Orlando]

You're preserving the location in the case that only one instruction is moved. Interestingly the linked document also says "a transformation should also preserve the debug location of an instruction that is moved between basic blocks, if the destination block already contains an instruction with an identical debug location." suggesting that you should only preserve the location if "an identical one" exists in the pred block. Scanning the block for an identical location sounds excessive. My gut reaction would be to simply add an additional check that the location of the instruction at the insertion point matches the (single) hoisted instruction's location before preserving the location, or alternatively always drop the location (i.e. remove the if guard altogether).

Digging deeper, it looks like this was discussed in the patch that introduces the wording to the document (D93662). cc @fhahn @vsk @jmorse. wdyt should happen here?

An aside: while looking at this I found a function Instruction::updateLocationAfterHoist() which looks like it might be useful, but all it does is call dropLocation().

[yuanboli233]

Thanks for your feedback @Orlando! I checked the code again, and I agree with your opinion, it seems that removing the if guard will be a conservative solution for this case.

[yuanboli233]

First updated to the version which simply drops the debug location, we may wait for other people's ideas.

[aprantl]

llvm/test/Transforms/GVNHoist/hoist_debugloc.ll

2

Could you add a comment explaining what this test is testing?

3

This test doesn't actually work for checking that there is no !dbg attachment on the instruction.
You probably want to add something like {{$}} at the end.

[yuanboli233]

I thought the previous CHECK will discard store instructions with debug location. Thanks for pointing it out @aprantl, I have added the comment and changed the CHECK. I have tested that the test case can now distinguish the versions before and after the proposed patch.

[jmorse]

Orlando wrote:

the linked document also says "a transformation should also preserve the debug location of an instruction that is moved between basic blocks, if the destination block already contains an instruction with an identical debug location."

Digging deeper, it looks like this was discussed in the patch that introduces the wording to the document (D93662). cc @fhahn @vsk @jmorse. wdyt should happen here?

IIRC, that wording means "It's sufficient to leave the DebugLoc unaltered if it's placed in a block where that DebugLoc already appears". It isn't necessary to scan for identical DebugLocs, instead it avoids un-necessary dropping of DebugLocs when we already know it won't mislead the developer.

llvm/lib/Transforms/Scalar/GVNHoist.cpp

1143–1146

Wording change suggestion above; I think it's clearer to just say "if instructions are hoisted". As I understand the HowToUpdate... document, we drop source locations whenever we move instructions between blocks, no matter how many.

IMO, it's better to not refer to the debug-info-update-guide from source comments -- the comments should only describe the intention / purpose of the code. (This may be a style thing).

llvm/test/Transforms/GVNHoist/hoist_debugloc.ll

4

You can hard-code the "G" global variable name, LLVM is highly unlikely to change that as part of a future optimisation.

36

Un-necessary attributes should be removed.

[yuanboli233]

Thank @jmorse for the suggestions, I have updated the diff to address them.

[jmorse]

LGTM

[jmorse]

NB: do you need this and D100270 to be pushed for you?

llvm/lib/Transforms/Scalar/GVNHoist.cpp

if (Repl) {
         // When not HoistingGeps we need to copy the GEPs.
         if (!makeGepOperandsAvailable(Repl, DestBB, InstructionsToHoist))
           continue;
       }
 
       // Move the instruction at the end of HoistPt.
       Instruction *Last = DestBB->getTerminator();
       MD->removeInstruction(Repl);
+      // If there are instructions to hoist, we should conservatively
+      // drop the debug location: if instructions are hoisted into a
+      // predecessor block, the source location should not be preserved.
+      Repl->dropLocation();
       Repl->moveBefore(Last);
 
       DFSNumber[Repl] = DFSNumber[Last]++;
     }
 
     NR += removeAndReplace(InstructionsToHoist, Repl, DestBB, MoveAccess);
 
     if (isa<LoadInst>(Repl))

llvm/test/Transforms/GVNHoist/hoist_debugloc.ll

+; RUN: opt -gvn-hoist %s -S 2>&1 | FileCheck %s
+; COM: Check whether the hoisted store instruction drops the debug location.
+; CHECK: store i32 1, i32* @G, align 4{{$}}
+source_filename = "abc.ll"
+
+@G = external global i32, align 4
+
+define void @foo(i32 %c1) !dbg !6 {
+entry:
+  call void @llvm.dbg.value(metadata i32 0, metadata !9, metadata !DIExpression()), !dbg !11
+  switch i32 %c1, label %exit1 [
+    i32 0, label %sw0
+    i32 1, label %sw1
+  ], !dbg !11
+
+sw0:                                              ; preds = %entry
+  store i32 1, i32* @G, align 4, !dbg !12
+  br label %exit, !dbg !13
+
+sw1:                                              ; preds = %entry
+  store i32 1, i32* @G, align 4, !dbg !14
+  br label %exit, !dbg !15
+
+exit1:                                            ; preds = %entry
+  store i32 1, i32* @G, align 4, !dbg !16
+  ret void, !dbg !17
+
+exit:                                             ; preds = %sw1, %sw0
+  ret void, !dbg !18
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+
+!llvm.dbg.cu = !{!0}
+!llvm.debugify = !{!3, !4}
+!llvm.module.flags = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "abc.ll", directory: "/")
+!2 = !{}
+!3 = !{i32 8}
+!4 = !{i32 1}
+!5 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+!7 = !DISubroutineType(types: !2)
+!8 = !{!9}
+!9 = !DILocalVariable(name: "1", scope: !6, file: !1, line: 1, type: !10)
+!10 = !DIBasicType(name: "ty32", size: 32, encoding: DW_ATE_unsigned)
+!11 = !DILocation(line: 1, column: 1, scope: !6)
+!12 = !DILocation(line: 2, column: 1, scope: !6)
+!13 = !DILocation(line: 3, column: 1, scope: !6)
+!14 = !DILocation(line: 4, column: 1, scope: !6)
+!15 = !DILocation(line: 5, column: 1, scope: !6)
+!16 = !DILocation(line: 6, column: 1, scope: !6)
+!17 = !DILocation(line: 7, column: 1, scope: !6)
+!18 = !DILocation(line: 8, column: 1, scope: !6)