[PATCH] D89818: [Compile Time] Make it possible to skip redundant debuginst removal

Wed Oct 21 12:51:10 PDT 2020

vsk added a comment.

In D89818#2344014 <https://reviews.llvm.org/D89818#2344014>, @bjope wrote:

> Comparing with the D88928 <https://reviews.llvm.org/D88928> patch (in its current state), that patch removes the RemoveRedundantDbgInstrs call from MergeBlockIntoPredecessor, but only replaces that with extra calls to RemoveRedundantDbgInstrs in a few places where MergeBlockIntoPredecessor is used (so afaict that patch currently removes a lot of DbgInstr cleanups?). IMO this patch makes it more of an active choice to decide if the removal can be deferred or not. With D88928 <https://reviews.llvm.org/D88928> it is easier to forget about such cleanups.

I did some digging (documented in D88928 <https://reviews.llvm.org/D88928>) about why we bother calling RemoveRedundantDbgInstrs in the first place, and couldn't find much besides a reference to llvm.org/PR35113. If it's a correctness concern, then we should the dbg inst cleanup behavior opt-out (that's what this patch does). If it's a compile-time concern, we should make the cleanup behavior opt-in, because doing the cleanup increases the worst-case complexity of MergeBlockIntoPredecessor.

> An alternative could be to mix the solutions a bit. We could have a version of MergeBlockIntoPredecessor that is doing the cleanup automatically, and one that skips the cleanup but instead returns the predecessor BB when a merge was made. The latter could be used in usecases such as the one in CodeGenPrepare, when we want to collect all the pred blocks in a set and defer calling of RemoveRedundantDbgInstrs for each of those blocks until after the inner loop.

Assuming that RemoveRedundantDbgInstrs is a compile-time fix: that sounds reasonable, but also more complicated. We might as well just have one version of the utility, and call RemoveRedundantDbgInstrs in cases where it demonstrably improves compile-time.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89818