[GVNHoist] Merge DebugLoc metadata on hoisted instructions

[twoh]

Summary

When instructions are hoisted, current implementation keeps DebugLoc metadata of the instruction that chosen as Repl (and its GEP operand if Repl is a load or a store). However, DebugLoc metadata should be updated to the 'merged' location across all hoisted instructions. See the following example code:

 1:  typedef struct {
 2:    int a[10];
 3:  } S1;
 4: 
 5:  extern S1 *s1[10];
 6: 
 7:  void foo(int x, int y, int i) {
 8:    if (y)
 9:      s1[i]->a[i] = x + y;
10:    else
11:      s1[i]->a[i] = x;
12:  }

Below is LLVM IR representation of the program before gvn-hoist:

%struct.S1 = type { [10 x i32] }
@s1 = external local_unnamed_addr global [10 x %struct.S1*], align 16

define void @foo(i32 %x, i32 %y, i32 %i) !dbg !4 {
entry:
  %tobool = icmp ne i32 %y, 0, !dbg !8
  br i1 %tobool, label %if.then, label %if.else, !dbg !10

if.then:                                          ; preds = %entry
  %add = add nsw i32 %x, %y, !dbg !11
  %idxprom = sext i32 %i to i64, !dbg !12
  %arrayidx = getelementptr inbounds [10 x %struct.S1*], [10 x %struct.S1*]* @s1, i64 0, i64 %idxprom, !dbg !12
  %0 = load %struct.S1*, %struct.S1** %arrayidx, align 8, !dbg !12, !tbaa !13
  %a = getelementptr inbounds %struct.S1, %struct.S1* %0, i32 0, i32 0, !dbg !17
  br label %if.end, !dbg !12

if.else:                                          ; preds = %entry
  %idxprom3 = sext i32 %i to i64, !dbg !18
  %arrayidx4 = getelementptr inbounds [10 x %struct.S1*], [10 x %struct.S1*]* @s1, i64 0, i64 %idxprom3, !dbg !18
  %1 = load %struct.S1*, %struct.S1** %arrayidx4, align 8, !dbg !18, !tbaa !13
  %a5 = getelementptr inbounds %struct.S1, %struct.S1* %1, i32 0, i32 0, !dbg !19
  br label %if.end

if.end:                                           ; preds = %if.else, %if.then
  %a5.sink = phi [10 x i32]* [ %a5, %if.else ], [ %a, %if.then ]
  %.sink = phi i32 [ %x, %if.else ], [ %add, %if.then ]
  %idxprom6 = sext i32 %i to i64
  %arrayidx7 = getelementptr inbounds [10 x i32], [10 x i32]* %a5.sink, i64 0, i64 %idxprom6
  store i32 %.sink, i32* %arrayidx7, align 4, !tbaa !20
  ret void, !dbg !22
}

where

!11 = !DILocation(line: 9, column: 18, scope: !9)
!12 = !DILocation(line: 9, column: 5, scope: !9)
!18 = !DILocation(line: 11, column: 5, scope: !9)
!19 = !DILocation(line: 11, column: 9, scope: !9)

. And below is after gvn-hoist:

define void @foo(i32 %x, i32 %y, i32 %i) !dbg !4 {
entry:
  %tobool = icmp ne i32 %y, 0, !dbg !8
  %idxprom = sext i32 %i to i64, !dbg !10
  %0 = getelementptr inbounds [10 x %struct.S1*], [10 x %struct.S1*]* @s1, i64 0, i64 %idxprom, !dbg !10
  %1 = load %struct.S1*, %struct.S1** %0, align 8, !dbg !10, !tbaa !11
  br i1 %tobool, label %if.then, label %if.else, !dbg !15

if.then:                                          ; preds = %entry
  %add = add nsw i32 %x, %y, !dbg !16
  %arrayidx = getelementptr inbounds [10 x %struct.S1*], [10 x %struct.S1*]* @s1, i64 0, i64 %idxprom, !dbg !10
  %a = getelementptr inbounds %struct.S1, %struct.S1* %1, i32 0, i32 0, !dbg !17
  br label %if.end, !dbg !10

if.else:                                          ; preds = %entry
  %arrayidx4 = getelementptr inbounds [10 x %struct.S1*], [10 x %struct.S1*]* @s1, i64 0, i64 %idxprom, !dbg !18
  %a5 = getelementptr inbounds %struct.S1, %struct.S1* %1, i32 0, i32 0, !dbg !19
  br label %if.end

if.end:                                           ; preds = %if.else, %if.then
  %a5.sink = phi [10 x i32]* [ %a5, %if.else ], [ %a, %if.then ]
  %.sink = phi i32 [ %x, %if.else ], [ %add, %if.then ]
  %arrayidx7 = getelementptr inbounds [10 x i32], [10 x i32]* %a5.sink, i64 0, i64 %idxprom
  store i32 %.sink, i32* %arrayidx7, align 4, !tbaa !20
  ret void, !dbg !22
}

As you see, loads and their GEPs have been hosited from if.then/if.else block to entry block. However, DebugLoc metadata of these new instructions are still same as the instructions in if.then block, as they are moved/cloned from if.then block. This may result incorrect stepping and imprecise sample profile result.

[sebpop]

Thanks Taewook for fixing this!
The patch looks good to me.
I cannot approve the patch, and I would like someone more knowledgeable with debug info to have a look as well.

[sebpop]

The change looks simple enough. Please commit.

[twoh]

@sebpop Thank you for your review!

lib/Transforms/Scalar/GVNHoist.cpp

 // 2. geps when corresponding load/store cannot be hoisted.
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Scalar/GVN.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/ValueTracking.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils/Local.h"
 #include "llvm/Transforms/Utils/MemorySSA.h"
 
 using namespace llvm;
 
 #define DEBUG_TYPE "gvn-hoist"
 
void makeGepsAvailable(Instruction *Repl, BasicBlock *HoistPt,
     for (const Instruction *OtherInst : InstructionsToHoist) {
       const GetElementPtrInst *OtherGep;
       if (auto *OtherLd = dyn_cast<LoadInst>(OtherInst))
         OtherGep = cast<GetElementPtrInst>(OtherLd->getPointerOperand());
       else
         OtherGep = cast<GetElementPtrInst>(
             cast<StoreInst>(OtherInst)->getPointerOperand());
       ClonedGep->andIRFlags(OtherGep);
+      ClonedGep->setDebugLoc(
+          DILocation::getMergedLocation(
+            ClonedGep->getDebugLoc(), OtherGep->getDebugLoc()));
     }
 
     // Replace uses of Gep with ClonedGep in Repl.
     Repl->replaceUsesOfWith(Gep, ClonedGep);
   }
 
   // In the case Repl is a load or a store, we make all their GEPs
   // available: GEPs are not hoisted by default to avoid the address
for (const HoistingPointInfo &HP : HPL) {
           if (NewMemAcc) {
             // Update the uses of the old MSSA access with NewMemAcc.
             MemoryAccess *OldMA = MSSA->getMemoryAccess(I);
             OldMA->replaceAllUsesWith(NewMemAcc);
             MSSA->removeMemoryAccess(OldMA);
           }
 
           Repl->andIRFlags(I);
+          Repl->setDebugLoc(
+              DILocation::getMergedLocation(
+                Repl->getDebugLoc(), I->getDebugLoc()));
           combineKnownMetadata(Repl, I);
           I->replaceAllUsesWith(Repl);
           // Also invalidate the Alias Analysis cache.
           MD->removeInstruction(I);
           I->eraseFromParent();
         }
 
       // Remove MemorySSA phi nodes with the same arguments.

test/Transforms/GVNHoist/hoist-debugloc.ll

+; RUN: opt < %s -gvn-hoist -S | FileCheck %s
+; CHECK: [[VREG:%[^ ]+]] = getelementptr{{.*}}@s1
+; CHECK-NOT: !dbg
+; CHECK-SAME: {{$}}
+; CHECK-NEXT: load{{.*}}[[VREG]]
+; CHECK-NOT: !dbg
+; CHECK-SAME: {{$}}
+
+target triple = "x86_64-unknown-linux-gnu"
+
+%struct.S1 = type { [10 x i32] }
+
+@s1 = external local_unnamed_addr global [10 x %struct.S1*], align 16
+
+define void @foo(i32 %x, i32 %y, i32 %i) !dbg !4 {
+entry:
+  %tobool = icmp ne i32 %y, 0, !dbg !8
+  br i1 %tobool, label %if.then, label %if.else, !dbg !10
+
+if.then:                                          ; preds = %entry
+  %add = add nsw i32 %x, %y, !dbg !11
+  %idxprom = sext i32 %i to i64, !dbg !12
+  %arrayidx = getelementptr inbounds [10 x %struct.S1*], [10 x %struct.S1*]* @s1, i64 0, i64 %idxprom, !dbg !12
+  %0 = load %struct.S1*, %struct.S1** %arrayidx, align 8, !dbg !12, !tbaa !13
+  %a = getelementptr inbounds %struct.S1, %struct.S1* %0, i32 0, i32 0, !dbg !17
+  br label %if.end, !dbg !12
+
+if.else:                                          ; preds = %entry
+  %idxprom3 = sext i32 %i to i64, !dbg !18
+  %arrayidx4 = getelementptr inbounds [10 x %struct.S1*], [10 x %struct.S1*]* @s1, i64 0, i64 %idxprom3, !dbg !18
+  %1 = load %struct.S1*, %struct.S1** %arrayidx4, align 8, !dbg !18, !tbaa !13
+  %a5 = getelementptr inbounds %struct.S1, %struct.S1* %1, i32 0, i32 0, !dbg !19
+  br label %if.end
+
+if.end:                                           ; preds = %if.else, %if.then
+  %a5.sink = phi [10 x i32]* [ %a5, %if.else ], [ %a, %if.then ]
+  %.sink = phi i32 [ %x, %if.else ], [ %add, %if.then ]
+  %idxprom6 = sext i32 %i to i64
+  %arrayidx7 = getelementptr inbounds [10 x i32], [10 x i32]* %a5.sink, i64 0, i64 %idxprom6
+  store i32 %.sink, i32* %arrayidx7, align 4, !tbaa !20
+  ret void, !dbg !22
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+!1 = !DIFile(filename: "foo.c", directory: "b/")
+!2 = !{i32 2, !"Dwarf Version", i32 4}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 7, type: !5, isLocal: false, isDefinition: true, scopeLine: 7, flags: DIFlagPrototyped, isOptimized: true, unit: !0)
+!5 = !DISubroutineType(types: !6)
+!6 = !{null, !7, !7, !7}
+!7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!8 = !DILocation(line: 8, column: 7, scope: !9)
+!9 = distinct !DILexicalBlock(scope: !4, file: !1, line: 8, column: 7)
+!10 = !DILocation(line: 8, column: 7, scope: !4)
+!11 = !DILocation(line: 9, column: 18, scope: !9)
+!12 = !DILocation(line: 9, column: 5, scope: !9)
+!13 = !{!14, !14, i64 0}
+!14 = !{!"any pointer", !15, i64 0}
+!15 = !{!"omnipotent char", !16, i64 0}
+!16 = !{!"Simple C/C++ TBAA"}
+!17 = !DILocation(line: 9, column: 9, scope: !9)
+!18 = !DILocation(line: 11, column: 5, scope: !9)
+!19 = !DILocation(line: 11, column: 9, scope: !9)
+!20 = !{!21, !21, i64 0}
+!21 = !{!"int", !15, i64 0}
+!22 = !DILocation(line: 12, column: 1, scope: !4)