[PATCH] D146267: [llvm] Handle duplicate call bases when applying branch funneling

Thu Mar 23 13:41:17 PDT 2023

tejohnson added inline comments.

================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1397

+      if (!CB.getParent()) {
+        // When finding devirtualizable calls, it's possible to find the same
----------------
leonardchan wrote:
> aeubanks wrote:
> > leonardchan wrote:
> > > tejohnson wrote:
> > > > I don't think this is safe since when we eraseFromParent the instruction is deleted. Can we track a different way?
> > > Updated to just add erased pointers to a set and check on future iterations if they're in the set. This way we don't have to deref them.
> > if the instruction is erased, can there be instructions created after that share the same pointer?
> Hmm, I would think yes, but I think for this specific case it doesn't matter since none of the newly created instructions in this loop get added back to `CSInfo.CallSites` so each of the CBs should only refer to CBs that were added earlier.
While I think this would work, given what you mentioned above, the following is I think preferable from a safety/clarity perspective: Change CallBases to a map from the orig CallBase to the new one. Keep this bit of code the same so it skips any repeats. After the loop walk the map and do all of the replacing / erasing. Would that work?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146267