[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 a comment.

> I don't see a downside to that approach, and it'll be beneficial for downstream targets to have D73534 <https://reviews.llvm.org/D73534> landed for good.

Strong +1



================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:158
+  if (MI->isCandidateForCallSiteEntry())
+    Parent->getParent()->eraseCallSiteInfo(MI);
   Parent->getParent()->DeleteMachineInstr(MI);
----------------
vsk wrote:
> djtodoro wrote:
> > 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()?
> > 
> I see something slightly different.
> 
> ReplaceTailWithBranchTo does already call eraseCallSiteInfo, but eraseCallSiteInfo doesn't appear to handle bundles:
> 
> ```
> frame #4: 0x00000001012fde01 clang-10`llvm::MachineFunction::DeleteMachineInstr(this=0x0000000107b67420, MI=0x00000001089620d0) at MachineFunction.cpp:386:3
>    383    // be triggered during the implementation of support for the
>    384    // call site info of a new architecture. If the assertion is triggered,
>    385    // back trace will tell where to insert a call to updateCallSiteInfo().
> -> 386    assert((!MI->isCandidateForCallSiteEntry() ||
>    387            CallSitesInfo.find(MI) == CallSitesInfo.end()) &&
>    388           "Call site info was not updated!");
>    389    // Strip it for parts. The operand array and the MI object itself are
> (lldb) p MI->dump()
>   UNKNOWN 1, killed $physreg3, @_ZdlPv, <regmask ...>, implicit-def dead $physreg13, implicit $physreg15, implicit internal killed $physreg72, implicit-def $physreg15, implicit internal killed $physreg72, implicit-def dead $physreg72, implicit internal killed $physreg12
> (lldb) p MI->isCandidateForCallSiteEntry()
> (bool) $0 = true
> 
> (lldb) up
> frame #9: 0x00000001015154f8 clang-10`llvm::TargetInstrInfo::ReplaceTailWithBranchTo(this=<unavailable>, Tail=<unavailable>, NewDest=0x000000010894ff28) const at TargetInstrInfo.cpp:147:10 [opt]
>    144      auto MI = Tail++;
>    145      if (MI->isCandidateForCallSiteEntry())
>    146        MBB->getParent()->eraseCallSiteInfo(&*MI);
> -> 147      MBB->erase(MI);
> ```
Yes, the bundle is the problem.

Hm, I agree, I should just add handling of bundles within eraseCallSiteInfo(), that will handle this. That is better idea.

I was occupied with the idea of removing the assert, and did not think about that alternative. I’ll update the patch :)


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

https://reviews.llvm.org/D74904