When hoisting instructions out of loops their debug loc should be removed (MLICM pass).

[wolfgangp]

Summary

The Machine Loop Invariant Code Motion pass (MLICM) hoists loop invariant instructions out of loops, e.g. address and other constant generating instructions. Retaining their debug location can cause erratic stepping behavior during debugging as well as incorrect source attribution wrt AutoFDO.
This patch proposes to remove debug locations for these instructions.

This is the first of 3 patches with the theme of instruction hoisting. I'm keeping them separate b/c 3 different passes are involved.

This is a case of true code motion, so retaining the original debug location is not justified.

[dblaikie]

Seems reasonable - any chance of simplifying the test case further?

lib/CodeGen/MachineLICM.cpp

1339 ↗

(On Diff #79844)

Might be worth a comment

[wolfgangp]

Simplified test case and added comment.

[wolfgangp]

In D27290#611005, @dblaikie wrote:

Seems reasonable - any chance of simplifying the test case further?

Thanks for the review, David. I simplified the test case and added a comment.

[dblaikie]

Right, Phab doesn't send zero content approvals - anyway, signed off. Carry on.

llvm/trunk/lib/CodeGen/MachineLICM.cpp

bool MachineLICM::Hoist(MachineInstr *MI, MachineBasicBlock *Preheader) {
   // Look for opportunity to CSE the hoisted instruction.
   unsigned Opcode = MI->getOpcode();
   DenseMap<unsigned, std::vector<const MachineInstr*> >::iterator
     CI = CSEMap.find(Opcode);
   if (!EliminateCSE(MI, CI)) {
     // Otherwise, splice the instruction to the preheader.
     Preheader->splice(Preheader->getFirstTerminator(),MI->getParent(),MI);
 
+    // Since we are moving the instruction out of its basic block, we do not
+    // retain its debug location. Doing so would degrade the debugging 
+    // experience and adversely affect the accuracy of profiling information.
+    MI->setDebugLoc(DebugLoc());
+
     // Update register pressure for BBs from header to this block.
     UpdateBackTraceRegPressure(MI);
 
     // Clear the kill flags of any register this instruction defines,
     // since they may need to be live throughout the entire loop
     // rather than just live for part of it.
     for (MachineOperand &MO : MI->operands())
       if (MO.isReg() && MO.isDef() && !MO.isDead())

llvm/trunk/test/CodeGen/ARM/debug-info-branch-folding.ll

 ; RUN: llc < %s - | FileCheck %s
 target datalayout = "e-p:32:32:32-i1:8:32-i8:8:32-i16:16:32-i32:32:32-i64:32:32-f32:32:32-f64:32:32-v64:32:64-v128:32:128-a0:0:32-n32"
 target triple = "thumbv7-apple-macosx10.6.7"
 
 ;CHECK:   vadd.f32  q4, q8, q8
-;CHECK-NEXT: Ltmp1
-;CHECK-NEXT: LBB0_1
+;CHECK:         LBB0_1:
+;CHECK-NOT:     beq LBB0_1
 
-;CHECK:@DEBUG_VALUE: x <- %Q4{{$}}
-;CHECK-NEXT:@DEBUG_VALUE: y <- %Q4{{$}}
+;CHECK:         @DEBUG_VALUE: x <- %Q4{{$}}
+;CHECK-NEXT:    @DEBUG_VALUE: y <- %Q4{{$}}
+;CHECK:         beq LBB0_1
 
 
 @.str = external constant [13 x i8]
 
 declare <4 x float> @test0001(float) nounwind readnone ssp
 
 define i32 @main(i32 %argc, i8** nocapture %argv, i1 %cond) nounwind ssp !dbg !10 {
 entry:

llvm/trunk/test/DebugInfo/MIR/X86/mlicm-hoist.mir

+--- |
+  ; RUN: llc -run-pass=machinelicm -o - %s | FileCheck %s
+  ; Line numbers should not be retained when loop invariant instructions are hoisted.
+  ; Doing so causes poor stepping bevavior.
+  ;
+  ; Created from:
+  ; int x;
+  ; void Process(const unsigned int *p)
+  ; {
+  ;    while (1)
+  ;      x = *p++;
+  ; }
+  ;
+  ; with clang -emit-llvm -S -O2 -g -o t.ll t.c
+  ;      llc -relocation-model=pic -stop-after=machine-combine -O2 t.ll -o t.mir
+  ;
+  ; We check that when the machine loop invariant pass hoists instructions
+  ; out of a loop the debug information is not retained.
+  ;
+  ; CHECK-LABEL:  bb.0.entry:
+  ; CHECK:        MOV64rm %rip, 1, _, target-flags(x86-gotpcrel) @x, _ :: (load 8 from got)
+  ; CHECK-LABEL:  bb.1.while.body:
+  ;
+  ; ModuleID = 'tx.ll'
+  source_filename = "t.c"
+  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+
+  @x = common local_unnamed_addr global i32 0, align 4, !dbg !0
+
+  ; Function Attrs: noreturn nounwind uwtable
+  define void @Process(i32* nocapture readonly %p) local_unnamed_addr #0 !dbg !9 {
+  entry:
+    tail call void @llvm.dbg.value(metadata i32* %p, i64 0, metadata !16, metadata !17), !dbg !18
+    br label %while.body, !dbg !19
+
+  while.body:                                       ; preds = %while.body, %entry
+    %p.addr.0 = phi i32* [ %p, %entry ], [ %incdec.ptr, %while.body ]
+    tail call void @llvm.dbg.value(metadata i32* %p.addr.0, i64 0, metadata !16, metadata !17), !dbg !18
+    %incdec.ptr = getelementptr inbounds i32, i32* %p.addr.0, i64 1, !dbg !20
+    tail call void @llvm.dbg.value(metadata i32* %incdec.ptr, i64 0, metadata !16, metadata !17), !dbg !18
+    %0 = load i32, i32* %p.addr.0, align 4, !dbg !21, !tbaa !22
+    store i32 %0, i32* @x, align 4, !dbg !26, !tbaa !22
+    br label %while.body, !dbg !27, !llvm.loop !29
+  }
+
+  ; Function Attrs: nounwind readnone
+  declare void @llvm.dbg.value(metadata, i64, metadata, metadata) #1
+
+  attributes #0 = { noreturn nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+  attributes #1 = { nounwind readnone }
+  attributes #2 = { nounwind }
+
+  !llvm.dbg.cu = !{!1}
+  !llvm.module.flags = !{!6, !7}
+  !llvm.ident = !{!8}
+
+  !0 = distinct !DIGlobalVariable(name: "x", scope: !1, file: !2, line: 1, type: !5, isLocal: false, isDefinition: true)
+  !1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2, producer: "clang version 4.0.0 ", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !3, globals: !4)
+  !2 = !DIFile(filename: "t.c", directory: "/home/test")
+  !3 = !{}
+  !4 = !{!0}
+  !5 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !6 = !{i32 2, !"Dwarf Version", i32 4}
+  !7 = !{i32 2, !"Debug Info Version", i32 3}
+  !8 = !{!"clang version 4.0.0 "}
+  !9 = distinct !DISubprogram(name: "Process", scope: !2, file: !2, line: 2, type: !10, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: true, unit: !1, variables: !15)
+  !10 = !DISubroutineType(types: !11)
+  !11 = !{null, !12}
+  !12 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !13, size: 64)
+  !13 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !14)
+  !14 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+  !15 = !{!16}
+  !16 = !DILocalVariable(name: "p", arg: 1, scope: !9, file: !2, line: 2, type: !12)
+  !17 = !DIExpression()
+  !18 = !DILocation(line: 2, column: 34, scope: !9)
+  !19 = !DILocation(line: 4, column: 3, scope: !9)
+  !20 = !DILocation(line: 5, column: 11, scope: !9)
+  !21 = !DILocation(line: 5, column: 9, scope: !9)
+  !22 = !{!23, !23, i64 0}
+  !23 = !{!"int", !24, i64 0}
+  !24 = !{!"omnipotent char", !25, i64 0}
+  !25 = !{!"Simple C/C++ TBAA"}
+  !26 = !DILocation(line: 5, column: 7, scope: !9)
+  !27 = !DILocation(line: 4, column: 3, scope: !28)
+  !28 = !DILexicalBlockFile(scope: !9, file: !2, discriminator: 1)
+  !29 = distinct !{!29, !19, !20}
+
+...
+---
+name:            Process
+alignment:       4
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gr64 }
+  - { id: 1, class: gr64 }
+  - { id: 2, class: gr64 }
+  - { id: 3, class: gr32 }
+  - { id: 4, class: gr64 }
+liveins:
+  - { reg: '%rdi', virtual-reg: '%2' }
+frameInfo:
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    0
+  adjustsStack:    false
+  hasCalls:        false
+  maxCallFrameSize: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+body:             |
+  bb.0.entry:
+    successors: %bb.1.while.body(0x80000000)
+    liveins: %rdi
+
+    DBG_VALUE debug-use %rdi, debug-use _, !16, !17, debug-location !18
+    %2 = COPY %rdi
+    DBG_VALUE debug-use %2, debug-use _, !16, !17, debug-location !18
+
+  bb.1.while.body:
+    successors: %bb.1.while.body(0x80000000)
+
+    %0 = PHI %2, %bb.0.entry, %1, %bb.1.while.body
+    DBG_VALUE debug-use %0, debug-use _, !16, !17, debug-location !18
+    %1 = ADD64ri8 %0, 4, implicit-def dead %eflags, debug-location !20
+    DBG_VALUE debug-use %1, debug-use _, !16, !17, debug-location !18
+    %3 = MOV32rm %0, 1, _, 0, _, debug-location !21 :: (load 4 from %ir.p.addr.0, !tbaa !22)
+    %4 = MOV64rm %rip, 1, _, target-flags(x86-gotpcrel) @x, _, debug-location !26 :: (load 8 from got)
+    MOV32mr killed %4, 1, _, 0, _, killed %3, debug-location !26 :: (store 4 into @x, !tbaa !22)
+    JMP_1 %bb.1.while.body, debug-location !27
+
+...