[DebugInfo][If-Converter] Update call site info during the optimization

[NikolaPrica]

Summary

During the If-Converter optimization pay attention when copying or deleting call instructions in order to keep call site information in valid state.

[vsk]

test/CodeGen/ARM/smml.ll

13

How did you catch this?

Have you run the test suite using "./bin/llvm-lit -Dllc="llc -debug-entry-values"?

[aprantl]

lib/CodeGen/IfConversion.cpp

1785

It looks like we will have to do something similar in a lot of places. Have you thought about a way to make this more automatic, for example by registering a hook on deleted instructions that gets called automatically (I don't know what kind of infrastructure we have, but I'm thinking of something akin to salvageDebugInfo())?

[vsk]

lib/CodeGen/IfConversion.cpp

1785

I've asked about this before. The rationale for weeding out issues with an assert is to a) avoid slowing down each MachineInstr::eraseFromParent call and b) allow for the possibility of preserving call site information on inst deletion rather than throwing it away. This makes sense to me: I'm satisfied that we have an assert that lets us find the places call site info needs to be updated.

[aprantl]

lib/CodeGen/IfConversion.cpp

1785

I was unaware (or forgot about) the assert. That's fine with me then!

[NikolaPrica]

test/CodeGen/ARM/smml.ll

13

I've built couple of binaries that triggered the assertion and then resolved it. Since I was testing for the ARM I just replaced updatedCallSiteInfo with an assertion and run all ARM tests to see whether it will trigger something. But the call you mentioned is simpler one! I will try it in future! Thanks!

[vsk]

FWIW, this looks good to me, but it would be great to get another +1 as I'm not familiar with this pass.

[NikolaPrica]

Should we look for another reviewer or this is good to go?

[efriedma]

lib/CodeGen/IfConversion.cpp

2071

This comment doesn't make sense; what's getting deleted?

[NikolaPrica]

Thanks for the comment @efriedma! I've looked more carefully this optimization. It turns out that after CopyAndPredicateBlock it is not guaranteed that copied instruction block will be deleted. The deletion of critical call instruction happens after If-conversion during the BranchFolding cleaning when some block does not have predecessors.

Since we are not sure whether the block will be deleted after CopyAndPredicateBlock I've extended updateCallSiteInfo to be able to just make a copy of call site info.

[efriedma]

include/llvm/CodeGen/MachineFunction.h

987

Can we split this API into multiple entry points? It now has three different modes with different semantics, which is more confusing than helpful.

[NikolaPrica]

Split call site updating functions on move, copy and erase functions.

[vsk]

Splitting up the updateCallSiteInfo API is a great idea. Some minor comments inline.

include/llvm/CodeGen/MachineFunction.h

992

nit: s/the//

997

nit: s/we are removing/to remove/

[NikolaPrica]

Adjust comments in MachineFunction.h

[efriedma]

LGTM

lib/CodeGen/BranchFolding.cpp

168

This looks fine, but did you mean to include it here?

[NikolaPrica]

lib/CodeGen/BranchFolding.cpp

168

Yes. I meant to include it here. The assertion from MachineFunction::DeleteMachineInstr was triggered here. With analysis I came to the conclusion that it is a consequence of removing predecessors of MachineBasicBlock after calling IfConverter::CopyAndPredicateBlock in IfConvertSimple or IfConvertTriangle optimizations. At some point certain MBB will not have any predecessors and thus it will be removed during the BranchFolding optimization which also removes dead blocks. Without this part of code test case "DebugInfo/MIR/ARM/if-coverter-call-site-info.mir" would trigger mentioned assertion.

[efriedma]

lib/CodeGen/BranchFolding.cpp

168

Ok, that makes sense.

include/llvm/CodeGen/MachineFunction.h

void addCallArgsForwardingRegs(const MachineInstr *CallI,
     assert(CallI->isCall());
     CallSitesInfo[CallI] = std::move(CallInfo);
   }
 
   const CallSiteInfoMap &getCallSitesInfo() const {
     return CallSitesInfo;
   }
 
-  /// Update call sites info by deleting entry for \p Old call instruction.
-  /// If \p New is present then transfer \p Old call info to it. This function
-  /// should be called before removing call instruction or before replacing
-  /// call instruction with new one.
+  /// This function should be called before removing, replacing or copying a
+  /// call instruction. Depending on the arguments provided, respective update
+  /// of a call site will be performed. If only \p Old is provided, the call
+  /// site info for that call instruction will be removed. If \p New is provided
+  /// then call site info from \p Old call instruction will be moved to \p New
+  /// call instruction call site info. The last parameter is used if we want to
+  /// copy call site info from \p Old call instruction to \p New.
   void updateCallSiteInfo(const MachineInstr *Old,
-                          const MachineInstr *New = nullptr);
+                          const MachineInstr *New = nullptr,
+                          bool PerformCopy = false);
 };
 
 //===--------------------------------------------------------------------===//
 // GraphTraits specializations for function basic block graphs (CFGs)
 //===--------------------------------------------------------------------===//
 
 // Provide specializations of GraphTraits to be able to treat a
 // machine function as a graph of machine basic blocks... these are
 // the same as the machine basic block iterators, except that the root
 // node is implicitly the first node of the function.
 //
 template <> struct GraphTraits<MachineFunction*> :
   public GraphTraits<MachineBasicBlock*> {
   static NodeRef getEntryNode(MachineFunction *F) { return &F->front(); }
 
   // nodes_iterator/begin/end - Allow iteration over all nodes in the graph

lib/CodeGen/BranchFolding.cpp

void BranchFolder::RemoveDeadBlock(MachineBasicBlock *MBB) {
   MachineFunction *MF = MBB->getParent();
   // drop all successors.
   while (!MBB->succ_empty())
     MBB->removeSuccessor(MBB->succ_end()-1);
 
   // Avoid matching if this pointer gets reused.
   TriedMerging.erase(MBB);
 
+  // Update call site info.
+  std::for_each(MBB->begin(), MBB->end(), [MF](const MachineInstr &MI) {
+    if (MI.isCall(MachineInstr::IgnoreBundle))
+      MF->updateCallSiteInfo(&MI);
+  });
   // Remove the block.
   MF->erase(MBB);
   EHScopeMembership.erase(MBB);
   if (MLI)
     MLI->removeBlock(MBB);
 }
 
 bool BranchFolder::OptimizeFunction(MachineFunction &MF,

lib/CodeGen/IfConversion.cpp

bool IfConverter::IfConvertDiamondCommon(
   // instruction, if it's not marked as "branch".
   for (unsigned i = 0; i < NumDups1; ++DI1) {
     if (DI1 == MBB1.end())
       break;
     if (!DI1->isDebugInstr())
       ++i;
   }
   while (NumDups1 != 0) {
+    // Since this instruction is going to be deleted, update call
+    // site info state if the instruction is call instruction.
+    if (DI2->isCall(MachineInstr::IgnoreBundle))
+      MBB2.getParent()->updateCallSiteInfo(&*DI2);
+
     ++DI2;
     if (DI2 == MBB2.end())
       break;
     if (!DI2->isDebugInstr())
       --NumDups1;
   }
 
   if (MRI->tracksLiveness()) {
while (DI1 != MBB1.begin()) {
     if (!Prev->isBranch() && !Prev->isDebugInstr())
       break;
     DI1 = Prev;
   }
   for (unsigned i = 0; i != NumDups2; ) {
     // NumDups2 only counted non-dbg_value instructions, so this won't
     // run off the head of the list.
     assert(DI1 != MBB1.begin());
+
     --DI1;
+
+    // Since this instruction is going to be deleted, update call
+    // site info state if the instruction is call instruction.
+    if (DI1->isCall(MachineInstr::IgnoreBundle))
+      MBB1.getParent()->updateCallSiteInfo(&*DI1);
+
     // skip dbg_value instructions
     if (!DI1->isDebugInstr())
       ++i;
   }
   MBB1.erase(DI1, MBB1.end());
 
   DI2 = BBI2->BB->end();
   // The branches have been checked to match. Skip over the branch in the false
void IfConverter::CopyAndPredicateBlock(BBInfo &ToBBI, BBInfo &FromBBI,
 
   MachineBasicBlock &FromMBB = *FromBBI.BB;
   for (MachineInstr &I : FromMBB) {
     // Do not copy the end of the block branches.
     if (IgnoreBr && I.isBranch())
       break;
 
     MachineInstr *MI = MF.CloneMachineInstr(&I);
+    // Make a copy of the call site info.
+    if (MI->isCall(MachineInstr::IgnoreBundle))
+      MF.updateCallSiteInfo(&I,MI, /*PerformCopy*/true);
+
     ToBBI.BB->insert(ToBBI.BB->end(), MI);
     ToBBI.NonPredSize++;
     unsigned ExtraPredCost = TII->getPredicationCost(I);
     unsigned NumCycles = SchedModel.computeInstrLatency(&I, false);
     if (NumCycles > 1)
       ToBBI.ExtraCost += NumCycles-1;
     ToBBI.ExtraCost2 += ExtraPredCost;
 

lib/CodeGen/MachineFunction.cpp

void MachineFunction::addCodeViewHeapAllocSite(MachineInstr *I,
   I->setPreInstrSymbol(*this, BeginLabel);
   I->setPostInstrSymbol(*this, EndLabel);
 
   const DIType *DI = dyn_cast<DIType>(MD);
   CodeViewHeapAllocSites.push_back(std::make_tuple(BeginLabel, EndLabel, DI));
 }
 
 void MachineFunction::updateCallSiteInfo(const MachineInstr *Old,
-                                         const MachineInstr *New) {
+                                         const MachineInstr *New,
+                                         bool PerformCopy) {
   if (!Target.Options.EnableDebugEntryValues || Old == New)
     return;
 
   assert(Old->isCall() && (!New || New->isCall()) &&
          "Call site info referes only to call instructions!");
   CallSiteInfoMap::iterator CSIt = CallSitesInfo.find(Old);
   if (CSIt == CallSitesInfo.end())
     return;
-  CallSiteInfo CSInfo = std::move(CSIt->second);
+  CallSiteInfo CSInfo = PerformCopy ? CSIt->second : std::move(CSIt->second);
+  if (!PerformCopy)
     CallSitesInfo.erase(CSIt);
   if (New)
     CallSitesInfo[New] = CSInfo;
 }
 
 /// \}
 
 //===----------------------------------------------------------------------===//
 //  MachineJumpTableInfo implementation

test/CodeGen/ARM/smml.ll

 ; RUN: llc -mtriple=arm-eabi %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK-V4
 ; RUN: llc -mtriple=armv6-eabi %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK-V6
 ; RUN: llc -mtriple=armv7-eabi %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK-V6
 ; RUN: llc -mtriple=thumb-eabi %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK-THUMB
 ; RUN: llc -mtriple=thumbv6-eabi %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK-THUMB
 ; RUN: llc -mtriple=thumbv6t2-eabi %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK-THUMBV6T2
 ; RUN: llc -mtriple=thumbv7-eabi %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK-THUMBV6T2
 ; RUN: llc -mtriple=thumbv7m-eabi %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK-V4
 ; RUN: llc -mtriple=thumbv7em-eabi %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK-THUMBV6T2
 
+; Next test would previously trigger an assertion responsible for verification of
+; call site info state.
+; RUN: llc -stop-after=if-converter -debug-entry-values -mtriple=thumbv6t2-eabi %s -o -| FileCheck %s -check-prefix=CHECK-CALLSITE
+; CHECK-CALLSITE: name:  test_used_flags
+; CHECK-CALLSITE: callSites:
+
+
 define i32 @Test0(i32 %a, i32 %b, i32 %c) nounwind readnone ssp {
 entry:
 ; CHECK-LABEL: Test0
 ; CHECK-NOT: smmls
   %conv4 = zext i32 %a to i64
   %conv1 = sext i32 %b to i64
   %conv2 = sext i32 %c to i64
   %mul = mul nsw i64 %conv2, %conv1

test/DebugInfo/MIR/ARM/if-coverter-call-site-info.mir

+# RUN: llc -mtriple=arm-linux-gnu -debug-entry-values -run-pass if-converter %s -o -| FileCheck %s
+
+# Vefify that the call site info will be updated after the optimization.
+# This test case would previously trigger an assertion when
+# deleting the call instruction.
+
+# Test case is generated from:
+# extern void
+# foo (int* seg, int subseg);
+# extern int* mri_common_symbol;
+# 
+# void
+# baa (int* secptr, int subseg)
+# {
+#   if (! (secptr == 0 && subseg == 0)) 
+#     foo (secptr, subseg);
+#   mri_common_symbol = 0;
+# }
+#
+# With slight change of MIR - substitution of BL instruction with BL_pred 
+# in order to trigger optimization.
+# clang -target arm-linux-gnu -g -O2 -Xclang -femit-debug-entry-values 
+#       %s -stop-before=if-convert
+#
+# CHECK: callSites:
+# CHECK-NEXT: - { bb: {{.*}}, offset: {{.*}}, fwdArgRegs: 
+# CHECK-NEXT:     - { arg: 0, reg: '$r0' }
+# CHECK-NEXT:     - { arg: 1, reg: '$r1' } }
+
+--- |
+  ; ModuleID = 'if-convert-call-site-info.c'
+  source_filename = "if-convert-call-site-info.c"
+  target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
+  target triple = "armv6kz-unknown-linux-gnueabihf"
+  
+  @mri_common_symbol = external dso_local local_unnamed_addr global i32*, align 4
+  
+  ; Function Attrs: nounwind
+  define dso_local void @baa(i32* %secptr, i32 %subseg) local_unnamed_addr #0 !dbg !14 {
+  entry:
+    call void @llvm.dbg.value(metadata i32* %secptr, metadata !16, metadata !DIExpression()), !dbg !18
+    call void @llvm.dbg.value(metadata i32 %subseg, metadata !17, metadata !DIExpression()), !dbg !18
+    %cmp = icmp eq i32* %secptr, null, !dbg !19
+    %cmp1 = icmp eq i32 %subseg, 0, !dbg !21
+    %or.cond = and i1 %cmp, %cmp1, !dbg !22
+    br i1 %or.cond, label %if.end, label %if.then, !dbg !22
+  
+  if.then:                                          ; preds = %entry
+    tail call void @foo(i32* %secptr, i32 %subseg), !dbg !23
+    br label %if.end, !dbg !23
+  
+  if.end:                                           ; preds = %entry, %if.then
+    store i32* null, i32** @mri_common_symbol, align 4, !dbg !24, !tbaa !25
+    ret void, !dbg !29
+  }
+  
+  declare !dbg !4 dso_local void @foo(i32*, i32) local_unnamed_addr
+  
+  ; Function Attrs: nounwind readnone speculatable willreturn
+  declare void @llvm.dbg.value(metadata, metadata, metadata)
+  
+  ; Function Attrs: nounwind
+  declare void @llvm.stackprotector(i8*, i8**)
+  
+  attributes #0 = { "frame-pointer"="all" }
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!9, !10, !11, !12}
+  !llvm.ident = !{!13}
+  
+  !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: "if-convert-call-site-info.c", directory: "/")
+  !2 = !{}
+  !3 = !{!4}
+  !4 = !DISubprogram(name: "foo", scope: !1, file: !1, line: 10, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+  !5 = !DISubroutineType(types: !6)
+  !6 = !{null, !7, !8}
+  !7 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !8, size: 32)
+  !8 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !9 = !{i32 2, !"Dwarf Version", i32 4}
+  !10 = !{i32 2, !"Debug Info Version", i32 3}
+  !11 = !{i32 1, !"wchar_size", i32 4}
+  !12 = !{i32 1, !"min_enum_size", i32 4}
+  !13 = !{!"clang version 10.0.0 "}
+  !14 = distinct !DISubprogram(name: "baa", scope: !1, file: !1, line: 14, type: !5, scopeLine: 15, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !15)
+  !15 = !{!16, !17}
+  !16 = !DILocalVariable(name: "secptr", arg: 1, scope: !14, file: !1, line: 14, type: !7, flags: DIFlagArgumentNotModified)
+  !17 = !DILocalVariable(name: "subseg", arg: 2, scope: !14, file: !1, line: 14, type: !8, flags: DIFlagArgumentNotModified)
+  !18 = !DILocation(line: 0, scope: !14)
+  !19 = !DILocation(line: 16, column: 17, scope: !20)
+  !20 = distinct !DILexicalBlock(scope: !14, file: !1, line: 16, column: 7)
+  !21 = !DILocation(line: 16, column: 32, scope: !20)
+  !22 = !DILocation(line: 16, column: 22, scope: !20)
+  !23 = !DILocation(line: 17, column: 5, scope: !20)
+  !24 = !DILocation(line: 18, column: 21, scope: !14)
+  !25 = !{!26, !26, i64 0}
+  !26 = !{!"any pointer", !27, i64 0}
+  !27 = !{!"omnipotent char", !28, i64 0}
+  !28 = !{!"Simple C/C++ TBAA"}
+  !29 = !DILocation(line: 19, column: 1, scope: !14)
+
+...
+---
+name:            baa
+alignment:       2
+tracksRegLiveness: true
+liveins:
+  - { reg: '$r0' }
+  - { reg: '$r1' }
+frameInfo:
+  stackSize:       8
+  maxAlignment:    4
+  adjustsStack:    true
+  hasCalls:        true
+  maxCallFrameSize: 0
+stack:
+  - { id: 0, type: spill-slot, offset: -4, size: 4, alignment: 4, callee-saved-register: '$lr', 
+      callee-saved-restored: false }
+  - { id: 1, type: spill-slot, offset: -8, size: 4, alignment: 4, callee-saved-register: '$r11' }
+callSites:
+  - { bb: 2, offset: 0, fwdArgRegs: 
+      - { arg: 0, reg: '$r0' }
+      - { arg: 1, reg: '$r1' } }
+constants:
+  - id:              0
+    value:           'i32** null'
+    alignment:       4
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    successors: %bb.1(0x60000000), %bb.2(0x20000000)
+    liveins: $r0, $r1, $lr
+  
+    DBG_VALUE $r0, $noreg, !16, !DIExpression(), debug-location !18
+    DBG_VALUE $r0, $noreg, !16, !DIExpression(), debug-location !18
+    DBG_VALUE $r1, $noreg, !17, !DIExpression(), debug-location !18
+    DBG_VALUE $r1, $noreg, !17, !DIExpression(), debug-location !18
+    $sp = frame-setup STMDB_UPD $sp, 14, $noreg, killed $r11, killed $lr
+    frame-setup CFI_INSTRUCTION def_cfa_offset 8
+    frame-setup CFI_INSTRUCTION offset $lr, -4
+    frame-setup CFI_INSTRUCTION offset $r11, -8
+    $r11 = frame-setup MOVr killed $sp, 14, $noreg, $noreg
+    frame-setup CFI_INSTRUCTION def_cfa_register $r11
+    CMPri renamable $r0, 0, 14, $noreg, implicit-def $cpsr, debug-location !22
+    Bcc %bb.2, 1, killed $cpsr, debug-location !22
+  
+  bb.1.entry:
+    successors: %bb.3(0x55555555), %bb.2(0x2aaaaaab)
+    liveins: $r0, $r1
+  
+    CMPri renamable $r1, 0, 14, $noreg, implicit-def $cpsr, debug-location !22
+    Bcc %bb.3, 0, killed $cpsr, debug-location !22
+  
+  bb.2.if.then:
+    liveins: $r0, $r1
+  
+    BL_pred @foo, 14, $noreg, csr_aapcs, implicit-def dead $lr, implicit $sp, implicit $r0, implicit $r1, implicit-def $sp, debug-location !23
+  
+  bb.3.if.end:
+    renamable $r0 = LDRi12 %const.0, 0, 14, $noreg, debug-location !24 :: (load 4 from constant-pool)
+    renamable $r1 = MOVi 0, 14, $noreg, $noreg
+    STRi12 killed renamable $r1, killed renamable $r0, 0, 14, $noreg, debug-location !24 :: (store 4 into @mri_common_symbol, !tbaa !25)
+    $sp = LDMIA_RET $sp, 14, $noreg, def $r11, def $pc, debug-location !29
+
+...