[CSInfo][TailDuplicator] Update the call site info

[djtodoro]

Summary

Erase the call site info when deleting the calls during the tail duplication.
This will fix the issue found when committing the D73534.

[vsk]

llvm/lib/CodeGen/TailDuplicator.cpp

739

Ditto, not sure why this isn't covered by the change in removeDeadBlock?

867

Why is this change necessary? I think this part of tailDuplicate just duplicates the contents of TailBB into PredBB. The deletion doesn't look like it happens until we get back to tailDuplicateAndUpdate, in removeDeadBlock.

If anything I'd expect that we'd have to copyCallSiteInfo here. I'll dig into this some more, I easily might be missing something.

1037

This makes sense to me.

[djtodoro]

llvm/lib/CodeGen/TailDuplicator.cpp

867

I think this won’t get into the removeDeadBlock part in some situations for particular calls. Probably we should use move* or copy* method here... I was struggling with making a test case for this, and had not payed enough attention on this part. I’ll double check this as well. Thanks!

[vsk]

FWIW, I applied this patch as well as D73534, then backed out the changes introduced here in duplicateSimpleBB and tailDuplicate, and got a clean check-llvm run (except for the test added here, which asserts that 'callSites' is empty). Did you notice some other test failing?

Note that we primarily use TailDuplicator::duplicateInstruction to copy instructions. That calls MachineFunction::CloneMachineInstr (via TargetInstrInfo::duplicate -> CloneMachineInstrBundle).

So perhaps we should just call MachineFunction::copyCallSiteInfo within CloneMachineInstr, if the original MI may have a call site entry?

[vsk]

Btw if you're interested in tackling that, perhaps it'd be best to split things into multiple patches (so, land the removeDeadBlock change from this patch, enable entry values, then keep on iterating)?

[djtodoro]

So perhaps we should just call MachineFunction::copyCallSiteInfo within CloneMachineInstr, if the original MI may have a call site entry?

This sounds good to me, but I'll investigate that to see if we should call the eraseCallSiteInfo as well.

Btw if you're interested in tackling that, perhaps it'd be best to split things into multiple patches (so, land the removeDeadBlock change from this patch, enable entry values, then keep on iterating)?

Nice, I agree.

[djtodoro]

• Split this into two parts

[djtodoro]

No need for this one. After the investigation I am realizing the moveCallSiteInfo() was properly called within X86ExpandPseudo::ExpandMI(), and handled the situation from this test case.

llvm/lib/CodeGen/TailDuplicator.cpp

for (MachineBasicBlock *PredBB : Preds) {
     SmallVector<MachineOperand, 4> PredCond;
     if (TII->analyzeBranch(*PredBB, PredTBB, PredFBB, PredCond))
       continue;
 
     Changed = true;
     LLVM_DEBUG(dbgs() << "\nTail-duplicating into PredBB: " << *PredBB
                       << "From simple Succ: " << *TailBB);
 
+    MachineFunction *MF = TailBB->getParent();
+    // Update the call site info.
+    std::for_each(TailBB->begin(), TailBB->end(), [MF](const MachineInstr &MI) {
+      if (MI.isCandidateForCallSiteEntry())
+        MF->eraseCallSiteInfo(&MI);
+    });
+
     MachineBasicBlock *NewTarget = *TailBB->succ_begin();
     MachineBasicBlock *NextBB = PredBB->getNextNode();
 
     // Make PredFBB explicit.
     if (PredCond.empty())
       PredFBB = PredTBB;
 
     // Make fall through explicit.
if (!(MF->getFunction().hasProfileData() && LayoutMode)) {
         IsLayoutSuccessor = true;
       if (IsLayoutSuccessor)
         continue;
     }
 
     LLVM_DEBUG(dbgs() << "\nTail-duplicating into PredBB: " << *PredBB
                       << "From Succ: " << *TailBB);
 
+    MachineFunction *MF = TailBB->getParent();
+    // Update the call site info.
+    std::for_each(TailBB->begin(), TailBB->end(), [MF](const MachineInstr &MI) {
+      if (MI.isCandidateForCallSiteEntry())
+        MF->eraseCallSiteInfo(&MI);
+    });
+
     TDBBs.push_back(PredBB);
 
     // Remove PredBB's unconditional branch.
     TII->removeBranch(*PredBB);
 
     // Clone the contents of TailBB into PredBB.
     DenseMap<unsigned, RegSubRegPair> LocalVRMap;
     SmallVector<std::pair<unsigned, RegSubRegPair>, 4> CopyInfos;
void TailDuplicator::removeDeadBlock(
   assert(MBB->pred_empty() && "MBB must be dead!");
   LLVM_DEBUG(dbgs() << "\nRemoving MBB: " << *MBB);
 
   MachineFunction *MF = MBB->getParent();
   // Update the call site info.
   std::for_each(MBB->begin(), MBB->end(), [MF](const MachineInstr &MI) {
     if (MI.isCandidateForCallSiteEntry())
       MF->eraseCallSiteInfo(&MI);
   });
 
   if (RemovalCallback)
     (*RemovalCallback)(MBB);
 
   // Remove all successors.
   while (!MBB->succ_empty())
     MBB->removeSuccessor(MBB->succ_end() - 1);
 
   // Remove the block.
   MBB->eraseFromParent();
 }

llvm/test/CodeGen/X86/call-site-update-taildup.ll

+; RUN: llc < %s -tail-dup-placement-threshold=4 -debug-entry-values=1 -stop-before=machineverifier | FileCheck %s
+
+;; Ensure that we removed the call site info during the tail duplication.
+;; The test was handmade.
+; CHECK: callSites: []
+
+; ModuleID = 'test.cpp'
+source_filename = "test.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define dso_local void @taildupit(i32* readonly %ptr_size) !dbg !8 {
+entry:
+  call void @llvm.dbg.value(metadata i32* %ptr_size, metadata !14, metadata !DIExpression()), !dbg !17
+  %tobool = icmp eq i32* %ptr_size, null, !dbg !17
+  br i1 %tobool, label %cond.end, label %cond.true, !dbg !17
+
+cond.true:                                        ; preds = %entry
+  %0 = load i32, i32* %ptr_size, align 4, !dbg !17
+  br label %cond.end, !dbg !17
+
+cond.end:                                         ; preds = %entry, %cond.true
+  %cond = phi i32 [ %0, %cond.true ], [ 1, %entry ], !dbg !17
+  %call = tail call i8* @f(i32 %cond), !dbg !17, !heapallocsite !2
+  call void @llvm.dbg.value(metadata i8* %call, metadata !15, metadata !DIExpression()), !dbg !17
+  tail call void @f2(), !dbg !17
+  ret void, !dbg !17
+}
+
+declare dso_local i8* @f(i32)
+
+declare dso_local void @f2()
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 11.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "test.cpp", directory: "/dir")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!8 = distinct !DISubprogram(name: "f1", linkageName: "f1", scope: !1, file: !1, line: 5, type: !9, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !13)
+!9 = !DISubroutineType(types: !10)
+!10 = !{null, !11}
+!11 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !12, size: 64)
+!12 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+!13 = !{!14, !15}
+!14 = !DILocalVariable(name: "ptr_size", arg: 1, scope: !8, file: !1, line: 5, type: !11)
+!15 = !DILocalVariable(name: "local", scope: !8, file: !1, line: 6, type: !16)
+!16 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 64)
+!17 = !DILocation(line: 0, scope: !8)