[PATCH] D73700: [CallSiteInfo] Fix the assertions regarding updating the CallSiteInfo

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:1373
+  if (MI->isCall())
+    MF.eraseCallSiteInfo(MI);
+
----------------
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?


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:1074
+    // Update the call site info.
+    if (MI->isCall())
+      MI->getMF()->moveCallSiteInfo(MI, &*MIB);
----------------
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`.


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:1095
+  // Update the call site info.
+  if (MI.isCall())
+    MI.getMF()->moveCallSiteInfo(&MI, &*MIB);
----------------
Ditto for PATCHABLE_EVENT_CALL, seems risky to emit call site info for this.


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:1115
+  // Update the call site info.
+  if (MI.isCall())
+    MI.getMF()->moveCallSiteInfo(&MI, &*MIB);
----------------
Ditto for PATCHABLE_TYPED_EVENT_CALL.


================
Comment at: llvm/test/CodeGen/AArch64/arm64-anyregcc.ll:3
+; in the valid state, an assert should be triggered.
+; RUN: llc < %s -debug-entry-values -mtriple=arm64-apple-darwin -stop-after=machineverifier | FileCheck %s -check-prefix=CHECK-CALLSITE
+; CHECK-CALLSITE: callSites:
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73700/new/

https://reviews.llvm.org/D73700