[DebugInfo] Remove call sites when eliminating unreachable blocks

[dstenb]

Summary

When eliminating an unreachable block we must remove any call site
information for calls residing in the block.

This was originally found on a downstream target, and the attached x86
test case was produced by hand-modifying some MIR.

[vsk]

Wdyt of addressing this problem in a more general way, e.g. in MachineInstruction::eraseFromParent itself?

edit: I meant to write MachineBasicBlock::eraseFromParent, but I think the point still stands.

[NikolaPrica]

@dstenb The patch looks good! LGTM! Thanks for working on this!

Wdyt of addressing this problem in a more general way, e.g. in MachineInstruction::eraseFromParent itself?
edit: I meant to write MachineBasicBlock::eraseFromParent, but I think the point still stands.

That would be great and we would not need to worry about validity of the CallSitesInfo. But the thing is that we have a problem with the MI range deletion from the MBB (see for example MachineOutliner::outline or version before this feature for TargetInstrInfo::ReplaceTailWithBranchTo). Such deletions cannot be generalized. For now, there are just two of such cases but in further we may stumble across more.
The whole idea behind this assertion and update is to evade generalization in the destructor of the MachineInst (which is the safest but requires certain overhead) and to use the update only when necessary. Similarly will be with the eraseFromParent, it will require certain overhead.
Please let us know if you have an idea how to overcome those constraints.

[vsk]

In D64500#1580217, @NikolaPrica wrote:

@dstenb The patch looks good! LGTM! Thanks for working on this!

Wdyt of addressing this problem in a more general way, e.g. in MachineInstruction::eraseFromParent itself?
edit: I meant to write MachineBasicBlock::eraseFromParent, but I think the point still stands.

That would be great and we would not need to worry about validity of the CallSitesInfo. But the thing is that we have a problem with the MI range deletion from the MBB (see for example MachineOutliner::outline or version before this feature for TargetInstrInfo::ReplaceTailWithBranchTo). Such deletions cannot be generalized. For now, there are just two of such cases but in further we may stumble across more.
The whole idea behind this assertion and update is to evade generalization in the destructor of the MachineInst (which is the safest but requires certain overhead) and to use the update only when necessary. Similarly will be with the eraseFromParent, it will require certain overhead.
Please let us know if you have an idea how to overcome those constraints.

Thanks for humoring me :). I haven't actively worked in this area, so I may be missing important/subtle details here.

But ISTM that range deletion within MachineBasicBlock can be handled generically as well ('if any MI in the range is a call ...'). It doesn't seem too far-fetched to teach all APIs that unlink an MI from the instruction list to update call site info. Perhaps this would incur a little extra overhead if, say, an MI is unlinked and subsequently re-inserted somewhere else. In this case there might be two updates instead of one. However, I'd argue that the cost is likely justified if it makes the Machine* APIs safe to use from a debug info correctness perspective. Has this already been tried, and been shown to be too expensive in compile-time benchmarking?

[dstenb]

I just want to add that I was a bit hesitant about generalizing the removal as I think it can be quite hard to tell when call site information has been removed, so I thought it would be better to have asserts trigger for each individual case, so that we can detect and assess what to do there, rather than dropping the information silently at the risk of false negatives.

The reason why I think it's hard to see that call site information has been removed is that, as far as I can tell there is no way to print the function attributes when doing something like -print-{before,after}-all, so it can be hard to to see where in the pass pipeline the callSites information was dropped. You could of course analyze the MIR and see where the information is likely to have been dropped, but I guess that can be difficult/annoying for larger programs.

However, perhaps there are no cases where we erase a block, and at the same would want to redirect call site information for calls inside that block so that it points to (newly created?) calls in other blocks. My concerns may be unfounded, and it should be fine to silently drop the call site information when erasing the block. I'm not sure though.

[vsk]

In D64500#1582335, @dstenb wrote:

I just want to add that I was a bit hesitant about generalizing the removal as I think it can be quite hard to tell when call site information has been removed, so I thought it would be better to have asserts trigger for each individual case, so that we can detect and assess what to do there, rather than dropping the information silently at the risk of false negatives.

I hadn't realized there was an assert in DeleteMachineInstr. That sounds like a reasonable/systematic way to find missed updates. That addresses my main concern :).

The reason why I think it's hard to see that call site information has been removed is that, as far as I can tell there is no way to print the function attributes when doing something like -print-{before,after}-all, so it can be hard to to see where in the pass pipeline the callSites information was dropped. You could of course analyze the MIR and see where the information is likely to have been dropped, but I guess that can be difficult/annoying for larger programs.

Oh, I thought r364506 (D61061) implemented MIR printer support for call site metadata. If not, it's probably worth adding.

However, perhaps there are no cases where we erase a block, and at the same would want to redirect call site information for calls inside that block so that it points to (newly created?) calls in other blocks. My concerns may be unfounded, and it should be fine to silently drop the call site information when erasing the block. I'm not sure though.

I think the current approach is fine. I don't yet see the need to pursue the alternative (updates in *::eraseFromParent).

[NikolaPrica]

The reason why I think it's hard to see that call site information has been removed is that, as far as I can tell there is no way to print the function attributes when doing something like -print-{before,after}-all, so it can be hard to to see where in the pass pipeline the callSites information was dropped. You could of course analyze the MIR and see where the information is likely to have been dropped, but I guess that can be difficult/annoying for larger programs.

Oh, I thought r364506 (D61061) implemented MIR printer support for call site metadata. If not, it's probably worth adding.

This dumps the CallSitesInfo when we producing a ".mir" file. The CallSitesInfo describes an internal state of the MF like the fixedStack, frameInfo etc. I'm not sure how we could make it recognizable in -print-{before,after}-all. Maybe like some metadata extension of call instruction. However, it should be part of another patch.

In D64500#1582335, @dstenb wrote:

However, perhaps there are no cases where we erase a block, and at the same would want to redirect call site information for calls inside that block so that it points to (newly created?) calls in other blocks. My concerns may be unfounded, and it should be fine to silently drop the call site information when erasing the block. I'm not sure though.

We share your concerns. We were not sure either. That's why we requested one more reconsideration after the patch was already accepted at D61062 and perhaps for the reasons you mentioned it is decided to go with assertion & addressing the missing parts.

Thanks for contribution!

[dstenb]

Thanks for the reviews, and sorry for taking some time to land this!

In D64500#1583215, @vsk wrote:

Oh, I thought r364506 (D61061) implemented MIR printer support for call site metadata. If not, it's probably worth adding.

Yes, that was a bit of a sloppy comment by me. I think that it would be easier if the information was visible when printing using the -print* options, but you can of course -stop-{before,after} your way through the passes to pinpoint where the information is dropped.

llvm/lib/CodeGen/UnreachableBlockElim.cpp

if (!Reachable.count(BB)) {
         }
 
         BB->removeSuccessor(BB->succ_begin());
       }
     }
   }
 
   // Actually remove the blocks now.
-  for (unsigned i = 0, e = DeadBlocks.size(); i != e; ++i)
+  for (unsigned i = 0, e = DeadBlocks.size(); i != e; ++i) {
+    // Remove any call site information for calls in the block.
+    for (auto &I : DeadBlocks[i]->instrs())
+      if (I.isCall(MachineInstr::IgnoreBundle))
+        DeadBlocks[i]->getParent()->updateCallSiteInfo(&I);
+
     DeadBlocks[i]->eraseFromParent();
+  }
 
   // Cleanup PHI nodes.
   for (MachineFunction::iterator I = F.begin(), E = F.end(); I != E; ++I) {
     MachineBasicBlock *BB = &*I;
     // Prune unneeded PHI entries.
     SmallPtrSet<MachineBasicBlock*, 8> preds(BB->pred_begin(),
                                              BB->pred_end());
     MachineBasicBlock::iterator phi = BB->begin();

llvm/test/DebugInfo/MIR/X86/unreachable-block-call-site.mir

+# RUN: llc -mtriple=x86_64-pc-linux -debug-entry-values -run-pass=unreachable-mbb-elimination -o - %s | FileCheck %s
+
+# Verify that the call site information for the call residing in the eliminated
+# block is removed. This test case would previously trigger an assertion when
+# deleting the call instruction.
+
+# CHECK: name: main
+# CHECK: callSites: []
+
+--- |
+  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+
+  ; Function Attrs: nounwind uwtable
+  define i32 @main() !dbg !12 {
+    unreachable
+  }
+
+  declare !dbg !4 void @foo()
+
+  !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 9.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, nameTableKind: None)
+  !1 = !DIFile(filename: "unreachable.c", directory: "/")
+  !2 = !{}
+  !3 = !{!4}
+  !4 = !DISubprogram(name: "foo", scope: !1, file: !1, line: 1, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+  !5 = !DISubroutineType(types: !6)
+  !6 = !{null}
+  !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 9.0.0"}
+  !12 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 3, type: !13, scopeLine: 3, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+  !13 = !DISubroutineType(types: !14)
+  !14 = !{!7}
+  !15 = distinct !DILexicalBlock(scope: !12, file: !1, line: 4, column: 7)
+  !16 = !DILocation(line: 4, column: 7, scope: !12)
+  !17 = !DILocation(line: 5, column: 5, scope: !15)
+  !18 = !DILocation(line: 6, column: 3, scope: !12)
+
+...
+---
+name:            main
+tracksRegLiveness: true
+callSites:
+  - { bb: 1, offset: 0 }
+body:             |
+  bb.0:
+    successors: %bb.2(0x80000000)
+
+    frame-setup PUSH64r undef $rax, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 16
+    JMP_1 %bb.2, debug-location !16
+
+  bb.1:
+    CALL64pcrel32 @foo, csr_64, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, debug-location !17
+
+  bb.2:
+    $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, debug-location !18
+    $rcx = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !18
+    CFI_INSTRUCTION def_cfa_offset 8, debug-location !18
+    RETQ killed $eax, debug-location !18
+
+...