[LiveDebugValues] Insert entry values after bundles

[dstenb]

Summary

Change LiveDebugValues so that it inserts entry values after the bundle
which contains the clobbering instruction. Previously it would insert
the debug value after the bundle head using insertAfter(), breaking the
bundle.

[dstenb]

llvm/lib/CodeGen/LiveDebugValues.cpp

1271–1272 ↗

(On Diff #217663)

A downstream comment was that this perhaps should become a helper function, e.g. MachineBasicBlock::insertAfterBundled().

[djtodoro]

@dstenb This is desirable! Thanks a lot for working on this! We also found such case when trying to add support for MIPS and we wanted to shared it, but it is good to see you also have found it!

I think MachineBasicBlock::insertAfterBundled() would be good to have.

[djtodoro]

llvm/test/DebugInfo/MIR/Hexagon/live-debug-values-bundled-entry-values.mir

46 ↗

(On Diff #217663)

version 10.0.0

57 ↗

(On Diff #217663)

version 10.0.0

[aprantl]

llvm/lib/CodeGen/LiveDebugValues.cpp

1271–1272 ↗

(On Diff #217663)

I think this would be much more readable indeed.

[aprantl]

otherwise lgtm

[NikolaPrica]

From my point of view this is good to go. 'insertAfterBundle' could go as NFC I suppose.

llvm/lib/CodeGen/LiveDebugValues.cpp

1271–1272 ↗

(On Diff #217663)

I would have expected from insertAfter to insert at the end of the bundle. Bundled instructions are usually inserted with MIBundlerBuilder. Comment for MachineBasicBlock::insert() indicates that MIBundleBuilder::insert is designed for prepending/appending to the bundle.

[dstenb]

Add MachineBasicBlock::insertAfterBundle() helper function.

[dstenb]

llvm/lib/CodeGen/LiveDebugValues.cpp

1271–1272 ↗

(On Diff #217663)

Perhaps that is how insertAfter() really ought to behave. Still, I think this new helper function can be useful since the iterator can point to any instruction in the bundle.

[vsk]

LGTM

llvm/trunk/include/llvm/CodeGen/MachineBasicBlock.h

#endif
   iterator insertAfter(iterator I, MachineInstr *MI) {
     assert((I == end() || I->getParent() == this) &&
            "iterator points outside of basic block");
     assert(!MI->isBundledWithPred() && !MI->isBundledWithSucc() &&
            "Cannot insert instruction with bundle flags");
     return Insts.insertAfter(I.getInstrIterator(), MI);
   }
 
+  /// If I is bundled then insert MI into the instruction list after the end of
+  /// the bundle, otherwise insert MI immediately after I.
+  instr_iterator insertAfterBundle(instr_iterator I, MachineInstr *MI) {
+    assert((I == instr_end() || I->getParent() == this) &&
+           "iterator points outside of basic block");
+    assert(!MI->isBundledWithPred() && !MI->isBundledWithSucc() &&
+           "Cannot insert instruction with bundle flags");
+    while (I->isBundledWithSucc())
+      ++I;
+    return Insts.insertAfter(I, MI);
+  }
+
   /// Remove an instruction from the instruction list and delete it.
   ///
   /// If the instruction is part of a bundle, the other instructions in the
   /// bundle will still be bundled after removing the single instruction.
   instr_iterator erase(instr_iterator I);
 
   /// Remove an instruction from the instruction list and delete it.
   ///

llvm/trunk/lib/CodeGen/LiveDebugValues.cpp

while (!Worklist.empty()) {
         OpenRanges.insertFromLocSet(PendingInLocs[MBB], VarLocIDs);
         for (auto &MI : *MBB)
               process(MI, OpenRanges, OutLocs, VarLocIDs, Transfers,
                       DebugEntryVals, OverlapFragments, SeenFragments);
         OLChanged |= transferTerminator(MBB, OpenRanges, OutLocs, VarLocIDs);
 
         // Add any DBG_VALUE instructions necessitated by spills.
         for (auto &TR : Transfers)
-          MBB->insertAfter(MachineBasicBlock::iterator(*TR.TransferInst),
-                           TR.DebugInst);
+          MBB->insertAfterBundle(TR.TransferInst->getIterator(), TR.DebugInst);
         Transfers.clear();
 
         LLVM_DEBUG(printVarLocInMBB(MF, OutLocs, VarLocIDs,
                                     "OutLocs after propagating", dbgs()));
         LLVM_DEBUG(printVarLocInMBB(MF, InLocs, VarLocIDs,
                                     "InLocs after propagating", dbgs()));
 
         if (OLChanged) {

llvm/trunk/test/DebugInfo/MIR/Hexagon/lit.local.cfg

+if not 'Hexagon' in config.root.targets:
+    config.unsupported = True

llvm/trunk/test/DebugInfo/MIR/Hexagon/live-debug-values-bundled-entry-values.mir

+# RUN: llc -debug-entry-values -run-pass=livedebugvalues -o - %s | FileCheck %s
+
+# Verify that the entry values for the input parameters are inserted after the
+# bundles which contains the registers' clobbering instructions (the calls to
+# @clobber). Previously the insertions would thrash the bundles, later on
+# triggering a crash in DebugHandlerBase.
+
+--- |
+  target datalayout = "e-m:e-p:32:32:32-a:0-n16:32-i64:64:64-i32:32:32-i16:16:16-i1:8:8-f32:32:32-f64:64:64-v32:32:32-v64:64:64-v512:512:512-v1024:1024:1024-v2048:2048:2048"
+  target triple = "hexagon"
+
+  ; Function Attrs: nounwind
+  define i32 @foo(i32 %arg) !dbg !12 {
+  entry:
+    call void @llvm.dbg.value(metadata i32 %arg, metadata !16, metadata !DIExpression()), !dbg !17
+    %call = tail call i32 @clobber(), !dbg !18
+    ret i32 1, !dbg !19
+  }
+
+  declare !dbg !4 i32 @clobber()
+
+  ; Function Attrs: nounwind
+  define i32 @bar(i32 %b) !dbg !20 {
+  entry:
+    call void @llvm.dbg.value(metadata i32 %b, metadata !22, metadata !DIExpression()), !dbg !23
+    %tobool = icmp eq i32 %b, 0, !dbg !24
+    br i1 %tobool, label %land.end, label %land.rhs, !dbg !24
+
+  land.rhs:                                         ; preds = %entry
+    %call = tail call i32 @clobber(), !dbg !25
+    br label %land.end
+
+  land.end:                                         ; preds = %entry, %land.rhs
+    ret i32 undef, !dbg !27
+  }
+
+  ; Function Attrs: nounwind readnone speculatable
+  declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+
+  attributes #0 = { nounwind readnone speculatable }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!8, !9, !10}
+  !llvm.ident = !{!11}
+
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 10.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, nameTableKind: None)
+  !1 = !DIFile(filename: "foo.c", directory: "/")
+  !2 = !{}
+  !3 = !{!4}
+  !4 = !DISubprogram(name: "clobber", scope: !1, file: !1, line: 1, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+  !5 = !DISubroutineType(types: !6)
+  !6 = !{!7}
+  !7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !8 = !{i32 2, !"Dwarf Version", i32 4}
+  !9 = !{i32 2, !"Debug Info Version", i32 3}
+  !10 = !{i32 1, !"wchar_size", i32 4}
+  !11 = !{!"clang version 10.0.0"}
+  !12 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 3, type: !13, scopeLine: 3, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !15)
+  !13 = !DISubroutineType(types: !14)
+  !14 = !{!7, !7}
+  !15 = !{!16}
+  !16 = !DILocalVariable(name: "arg", arg: 1, scope: !12, file: !1, line: 3, type: !7, flags: DIFlagArgumentNotModified)
+  !17 = !DILocation(line: 0, scope: !12)
+  !18 = !DILocation(line: 4, scope: !12)
+  !19 = !DILocation(line: 5, scope: !12)
+  !20 = distinct !DISubprogram(name: "bar", scope: !1, file: !1, line: 8, type: !13, scopeLine: 8, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !21)
+  !21 = !{!22}
+  !22 = !DILocalVariable(name: "arg", arg: 1, scope: !20, file: !1, line: 8, type: !7, flags: DIFlagArgumentNotModified)
+  !23 = !DILocation(line: 0, scope: !20)
+  !24 = !DILocation(line: 9, scope: !20)
+  !25 = !DILocation(line: 9, scope: !26)
+  !26 = distinct !DILexicalBlock(scope: !20, file: !1, line: 9)
+  !27 = !DILocation(line: 10, scope: !20)
+
+...
+---
+name:            foo
+tracksRegLiveness: true
+body:             |
+  bb.0.entry:
+    DBG_VALUE $r0, $noreg, !16, !DIExpression(), debug-location !17
+    CFI_INSTRUCTION def_cfa $r30, 8
+    CFI_INSTRUCTION offset $r31, -4
+    CFI_INSTRUCTION offset $r30, -8
+    BUNDLE implicit-def $r29, implicit-def $r30, implicit-def dead $pc, implicit-def dead $r31, implicit-def dead $r0, implicit $r29, implicit killed $framekey, implicit killed $framelimit, implicit killed $r30, implicit killed $r31, debug-location !18 {
+      $r29 = S2_allocframe $r29, 0, implicit-def $r30, implicit killed $framekey, implicit killed $framelimit, implicit killed $r30, implicit killed $r31, debug-location !18 :: (store 4 into stack)
+      J2_call @clobber, hexagoncsr, implicit-def dead $pc, implicit-def dead $r31, implicit internal killed $r29, implicit-def $r29, implicit-def dead $r0, debug-location !18
+    }
+    BUNDLE implicit-def dead $r0, implicit-def $d15, implicit-def $r30, implicit-def $r31, implicit-def $pc, implicit-def $r29, implicit killed $r30, implicit killed $framekey, debug-location !19 {
+      $r0 = A2_tfrsi 1, debug-location !19
+      $d15 = L4_return killed $r30, implicit-def $pc, implicit-def $r29, implicit killed $framekey, implicit-def dead $pc, implicit internal killed $r0, debug-location !19
+    }
+
+...
+
+# CHECK-LABEL: name: foo
+# CHECK: BUNDLE
+# CHECK-NEXT: $r29 = S2_allocframe $r29, 0
+# CHECK-NEXT: J2_call @clobber
+# CHECK-NEXT: }
+# CHECK-NEXT: DBG_VALUE $r0, $noreg, !16, !DIExpression(DW_OP_entry_value, 1)
+
+---
+name:            bar
+tracksRegLiveness: true
+liveins:
+  - { reg: '$r0' }
+body:             |
+  bb.0.entry:
+    successors: %bb.1
+    liveins: $r0
+
+    DBG_VALUE $r0, $noreg, !22, !DIExpression(), debug-location !23
+    DBG_VALUE $r0, $noreg, !22, !DIExpression(), debug-location !23
+    BUNDLE implicit-def dead $p0, implicit-def $pc, implicit killed $r0, implicit killed $r31, debug-location !24 {
+      renamable $p0 = C2_cmpeqi killed renamable $r0, 0, debug-location !24
+      PS_jmprettnew internal killed $p0, killed $r31, implicit-def $pc, implicit-def $pc, implicit killed undef $r0, debug-location !27
+    }
+
+  bb.1.land.rhs:
+    CFI_INSTRUCTION def_cfa $r30, 8
+    CFI_INSTRUCTION offset $r31, -4
+    CFI_INSTRUCTION offset $r30, -8
+    BUNDLE implicit-def $r29, implicit-def $r30, implicit-def $pc, implicit-def $r31, implicit-def dead $r0, implicit $r29, implicit killed $framekey, implicit killed $framelimit, implicit killed $r30, implicit killed $r31, debug-location !25 {
+      $r29 = S2_allocframe $r29, 0, implicit-def $r30, implicit killed $framekey, implicit killed $framelimit, implicit killed $r30, implicit killed $r31, debug-location !25 :: (store 4 into stack)
+      J2_call @clobber, hexagoncsr, implicit-def $pc, implicit-def $r31, implicit internal killed $r29, implicit-def $r29, implicit-def dead $r0, debug-location !25
+    }
+    $d15 = L2_deallocframe killed $r30, implicit-def $r29, implicit killed $framekey
+    PS_jmpret killed $r31, implicit-def $pc, implicit killed undef $r0, debug-location !27
+
+...
+
+# CHECK-LABEL: name: bar
+# CHECK: BUNDLE
+# CHECK: BUNDLE
+# CHECK-NEXT: $r29 = S2_allocframe $r29, 0
+# CHECK-NEXT: J2_call @clobber
+# CHECK-NEXT: }
+# CHECK-NEXT: DBG_VALUE $r0, $noreg, !22, !DIExpression(DW_OP_entry_value, 1)