[PATCH] D64500: [DebugInfo] Remove call sites when eliminating unreachable blocks

Fri Jul 12 11:25:07 PDT 2019

vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

In D64500#1582335 <https://reviews.llvm.org/D64500#1582335>, @dstenb wrote:

> I just want to add that I was a bit hesitant about generalizing the removal as I think it can be quite hard to tell when call site information has been removed, so I thought it would be better to have asserts trigger for each individual case, so that we can detect and assess what to do there, rather than dropping the information silently at the risk of false negatives.

I hadn't realized there was an assert in DeleteMachineInstr. That sounds like a reasonable/systematic way to find missed updates. That addresses my main concern :).

> The reason why I think it's hard to see that call site information has been removed is that, as far as I can tell there is no way to print the function attributes when doing something like `-print-{before,after}-all`, so it can be hard to to see where in the pass pipeline the `callSites` information was dropped. You could of course analyze the MIR and see where the information is likely to have been dropped, but I guess that can be difficult/annoying for larger programs.

Oh, I thought r364506 (D61061 <https://reviews.llvm.org/D61061>) implemented MIR printer support for call site metadata. If not, it's probably worth adding.

> However, perhaps there are no cases where we erase a block, and at the same would want to redirect call site information for calls inside that block so that it points to (newly created?) calls in other blocks. My concerns may be unfounded, and it should be fine to silently drop the call site information when erasing the block. I'm not sure though.

I think the current approach is fine. I don't yet see the need to pursue the alternative (updates in *::eraseFromParent).

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64500