[PATCH] D74904: WIP: [CallSiteInfo] Erase call site info for a MI from bundle

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked an inline comment as done.
djtodoro added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:158
+  if (MI->isCandidateForCallSiteEntry())
+    Parent->getParent()->eraseCallSiteInfo(MI);
   Parent->getParent()->DeleteMachineInstr(MI);
----------------
vsk wrote:
> I don't think we should erase call site info in deleteNode. Part of the reason the assert in DeleteMachineInstr was added, IIUC, was to help identify the areas where llvm should move or copy call site info. I fear that if we change deleteNode like this, we'd lose the ability to do that.
> 
> Could we address the issue in P8199 by call eraseCallSiteInfo in TargetInstrInfo::ReplaceTailWithBranchTo instead?
The eraseCallSiteInfo() was not called within the ReplaceTailWithBranchTo() (but there is a piece of code for that), since we started erasing an instruction that is not the candidate for call site entry, and then when we started erasing the instruction (actually the instr range starting from the MI) with erase(), a call site candidate occurred. So, that is the reason I put that within deleteNode() (there were no other functions called after the ReplaceTailWithBranchTo()).

I agree that we break the idea with the assert, but it might be more acceptable to put an assertion in CloneMachineInstr() and places like that, rather than in DeleteMachineInstr()?



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

https://reviews.llvm.org/D74904