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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

At this point, what I think we should do is:

1. Disable the assert in DeleteMachineInstr and file a PR to re-enable it.
2. Land D73534 <https://reviews.llvm.org/D73534>.
3. Once we're sure D73534 <https://reviews.llvm.org/D73534> has "stuck" in tree, iterate to re-enable the assert.

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.



================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:158
+  if (MI->isCandidateForCallSiteEntry())
+    Parent->getParent()->eraseCallSiteInfo(MI);
   Parent->getParent()->DeleteMachineInstr(MI);
----------------
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);
```


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

https://reviews.llvm.org/D74904