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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 2 inline comments as done.
djtodoro added a comment.

@vsk Thanks for the comments!



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


================
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:
----------------
vsk wrote:
> 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.
I thought removing them in the next patch from the stack. These should just show the places where the assertions have occurred. 


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

https://reviews.llvm.org/D73700