[CallSiteInfo] Fix the assertions regarding updating the CallSiteInfo

[djtodoro]

Summary

The call site info was not updated correctly when deleting corresponding call instructions.

(this was split from the D73534)

[vsk]

llvm/lib/CodeGen/MachineLICM.cpp

1373

I'm a bit confused by this one. ExtractHoistableLoad can hoist a call? What cases MI->isDereferenceableInvariantLoad(AA) and MI->isCall() to both be true?

llvm/lib/CodeGen/TargetLoweringBase.cpp

1074 ↗

(On Diff #241425)

Should PATCHPOINT/STATEPOINT/STACKMAP instructions ever be assigned call site info in the first place? It seems like these should not even get call site entries.

Maybe we could add a helper, like MachineInstr::isCandidateForCallSiteEntry(), that we can check before calling addCallArgsForwardingRegs.

1095 ↗

(On Diff #241425)

Ditto for PATCHABLE_EVENT_CALL, seems risky to emit call site info for this.

1115 ↗

(On Diff #241425)

Ditto for PATCHABLE_TYPED_EVENT_CALL.

llvm/test/CodeGen/AArch64/arm64-anyregcc.ll

3 ↗

(On Diff #241425)

It seems like the FileCheck commands added to these tests can just be removed, as they aren't strictly checking the 'callSites' info in the yaml. Without the FileCheck invocation, the test will still assert if assertions are enabled.

[djtodoro]

@vsk Thanks for the comments!

llvm/lib/CodeGen/TargetLoweringBase.cpp

1074 ↗

(On Diff #241425)

Maybe we could add a helper, like MachineInstr::isCandidateForCallSiteEntry(), that we can check before calling addCallArgsForwardingRegs.

We have an assertion within the addCallArgsForwardingRegs() checking the call site info is created only for the calls, but it turns out we have calls we do not want to handle here...

I agree, the MachineInstr::isCandidateForCallSiteEntry() is great idea to get this things into the good shape!
I will work on adding that!

llvm/test/CodeGen/AArch64/arm64-anyregcc.ll

3 ↗

(On Diff #241425)

I thought removing them in the next patch from the stack. These should just show the places where the assertions have occurred.

[djtodoro]

llvm/lib/CodeGen/MachineLICM.cpp

1373

I was surprised as well.

The case I found this:
CALL64m $rip, 1, $noreg, target-flags(x86-gotpcrel) @objc_msgSend, $noreg, <regmask $bh $bl $bp $bph $bpl $bx $ebp $ebx $hbp $hbx $rbp $rbx $r12 $r13 $r14 $r15 $r12b $r13b $r14b $r15b $r12bh $r13bh $r14bh $r15bh $r12d $r13d $r14d $r15d $r12w $r13w $r14w $r15w $r12wh and 3 more...>, implicit $rsp, implicit $ssp, implicit $rdi, implicit $rsi, implicit-def $rsp, implicit-def $ssp, implicit-def $rax :: (load 8 from got)

from the llvm/test/CodeGen/X86/hoist-invariant-load.ll

[djtodoro]

-Using the isCandidateForCallSiteEntry()

[vsk]

LGTM, pending resolution of https://reviews.llvm.org/D74121 vs. https://reviews.llvm.org/D74159. (I believe this can easily be rebased on top of either patch.)

[djtodoro]

-Rebase

llvm/lib/CodeGen/MachineLICM.cpp

if (!IsLoopInvariantInst(*NewMIs[0]) || !IsProfitableToHoist(*NewMIs[0])) {
     NewMIs[1]->eraseFromParent();
     return nullptr;
   }
 
   // Update register pressure for the unfolded instruction.
   UpdateRegPressure(NewMIs[1]);
 
   // Otherwise we successfully unfolded a load that we can hoist.
+
+  // Update the call site info.
+  if (MI->isCandidateForCallSiteEntry())
+    MF.eraseCallSiteInfo(MI);
+
   MI->eraseFromParent();
   return NewMIs[0];
 }
 
 /// Initialize the CSE map with instructions that are in the current loop
 /// preheader that may become duplicates of instructions that are hoisted
 /// out of the loop.
 void MachineLICMBase::InitCSEMap(MachineBasicBlock *BB) {

llvm/lib/Target/AArch64/AArch64CleanupLocalDynamicTLSPass.cpp

MachineInstr *replaceTLSBaseAddrCall(MachineInstr &I,
     const TargetInstrInfo *TII = MF->getSubtarget().getInstrInfo();
 
     // Insert a Copy from TLSBaseAddrReg to x0, which is where the rest of the
     // code sequence assumes the address will be.
     MachineInstr *Copy = BuildMI(*I.getParent(), I, I.getDebugLoc(),
                                  TII->get(TargetOpcode::COPY), AArch64::X0)
                              .addReg(TLSBaseAddrReg);
 
+    // Update the call site info.
+    if (I.isCandidateForCallSiteEntry())
+      I.getMF()->eraseCallSiteInfo(&I);
+
     // Erase the TLS_base_addr instruction.
     I.eraseFromParent();
 
     return Copy;
   }
 
   // Create a virtual register in *TLSBaseAddrReg, and populate it by
   // inserting a copy instruction after I. Returns the new instruction.

llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp

for (auto MII = MBB.instr_begin(), MIE = MBB.instr_end(); MII != MIE;) {
             TII->unfoldMemoryOperand(MF, MI, Reg, /*UnfoldLoad*/ true,
                                      /*UnfoldStore*/ false, NewMIs);
         (void)Unfolded;
         assert(Unfolded &&
                "Computed unfolded register class but failed to unfold");
         // Now stitch the new instructions into place and erase the old one.
         for (auto *NewMI : NewMIs)
           MBB.insert(MI.getIterator(), NewMI);
+
+        // Update the call site info.
+        if (MI.isCandidateForCallSiteEntry())
+          MF.eraseCallSiteInfo(&MI);
+
         MI.eraseFromParent();
         LLVM_DEBUG({
           dbgs() << "Unfolded load successfully into:\n";
           for (auto *NewMI : NewMIs) {
             NewMI->dump();
             dbgs() << "\n";
           }
         });

llvm/test/CodeGen/AArch64/arm64-tls-dynamics.ll

+; Verify the call site info. If the call site info is not
+; in the valid state, an assert should be triggered.
+; RUN: llc < %s -debug-entry-values -mtriple=arm64-none-linux-gnu -stop-after=machineverifier -relocation-model=pic -aarch64-elf-ldtls-generation=1 < %s
+
 ; RUN: llc -mtriple=arm64-none-linux-gnu -relocation-model=pic -aarch64-elf-ldtls-generation=1 -verify-machineinstrs < %s | FileCheck %s
 ; RUN: llc -mtriple=arm64-none-linux-gnu -relocation-model=pic -aarch64-elf-ldtls-generation=1 -filetype=obj < %s | llvm-objdump -r - | FileCheck --check-prefix=CHECK-RELOC %s
 ; RUN: llc -mtriple=arm64-none-linux-gnu -relocation-model=pic -verify-machineinstrs < %s | FileCheck --check-prefix=CHECK-NOLD %s
 ; RUN: llc -mtriple=arm64-none-linux-gnu -relocation-model=pic -filetype=obj < %s | llvm-objdump -r - | FileCheck --check-prefix=CHECK-NOLD-RELOC %s
 ; FIXME: We currently produce "small" code for the tiny model
 ; RUN: llc -mtriple=arm64-none-linux-gnu -relocation-model=pic -aarch64-elf-ldtls-generation=1 -code-model=tiny -verify-machineinstrs < %s | FileCheck %s
 ; FIXME: We currently error for the large code model
 ; RUN: not llc -mtriple=arm64-none-linux-gnu -relocation-model=pic -aarch64-elf-ldtls-generation=1 -code-model=large -verify-machineinstrs < %s 2>&1 | FileCheck %s --check-prefix=CHECK-LARGE

llvm/test/CodeGen/X86/hoist-invariant-load.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+
+; Verify the call site info. If the call site info is not
+; in the valid state, an assert should be triggered.
+; RUN: llc < %s -debug-entry-values -stop-after=machineverifier
+
 ; REQUIRES: asserts
 ; RUN: llc -mcpu=haswell < %s -O2 2>&1 | FileCheck %s
 ; For test:
 ; 2 invariant loads, 1 for OBJC_SELECTOR_REFERENCES_
 ; and 1 for objc_msgSend from the GOT
 ; For test_multi_def:
 ; 2 invariant load (full multiply, both loads should be hoisted.)
 ; For test_div_def:

llvm/test/CodeGen/X86/speculative-load-hardening-indirect.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+
+; Verify the call site info. If the call site info is not
+; in the valid state, an assert should be triggered.
+; RUN: llc < %s -debug-entry-values -mtriple=x86_64-unknown-linux-gnu -x86-speculative-load-hardening -stop-after=machineverifier
+
 ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-speculative-load-hardening -data-sections | FileCheck %s --check-prefix=X64
 ; FIXME: Fix machine verifier issues and remove -verify-machineinstrs=0. PR39451.
 ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-speculative-load-hardening -relocation-model pic -data-sections -verify-machineinstrs=0 | FileCheck %s --check-prefix=X64-PIC
 ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-speculative-load-hardening -data-sections -mattr=+retpoline | FileCheck %s --check-prefix=X64-RETPOLINE
 ;
 ; FIXME: Add support for 32-bit.
 
 @global_fnptr = external global i32 ()*