[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

1779

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

1779

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

1779

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

2065

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 ↗

(On Diff #220107)

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

996 ↗

(On Diff #220477)

nit: s/the//

1001 ↗

(On Diff #220477)

nit: s/we are removing/to remove/

[NikolaPrica]

Adjust comments in MachineFunction.h

[efriedma]

LGTM

lib/CodeGen/BranchFolding.cpp

168 ↗

(On Diff #220641)

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

[NikolaPrica]

lib/CodeGen/BranchFolding.cpp

168 ↗

(On Diff #220641)

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 ↗

(On Diff #220641)

Ok, that makes sense.

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()) {

 #endif
   BBI1->NonPredSize -= TII->removeBranch(*BBI1->BB);
   // Remove duplicated instructions.
   DI1 = MBB1.end();
   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);
+    // Since copied instruction is going to be deleted, update call
+    // site info state if the instruction is call instruction.
+    if (MI->isCall(MachineInstr::IgnoreBundle))
+      MF.updateCallSiteInfo(&I,MI);
+
     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;
 

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
+
+...