[PATCH] D88928: [Utils] Skip RemoveRedundantDbgInstrs in MergeBlockIntoPredecessor (PR47746)

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

In D88928#2316235 <https://reviews.llvm.org/D88928#2316235>, @bjope wrote:

> In D88928#2316194 <https://reviews.llvm.org/D88928#2316194>, @fhahn wrote:
>
>>> So: skip the call to RemoveRedundantDbgInstrs. There's surprisingly little fallout from this, and most of it can be addressed by doing RemoveRedundantDbgInstrs later.
>>
>> As an alternative, would it make sense to have a dedicated pass to eliminate redundant debug instructions, that's run late in the pipeline? not sure if running it once, but for every function will be better or worse in the end. It should be easy to skip for functions without debug info though.
>
> I don't remember exactly, but isn't one goal with removing the redundant dbg instructions early to speed up compilation (when -g is used). It's like saying that we shouldn't remove dead code until late in the pipeline to me (spending time on optimizing the dead code all the way through opt).

Right.

> Looks like it has been unfortunate to do the RemoveRedundantDbgInstrs inside MergeBlockIntoPredecessor for the CodeGenPrepare use-case, as it can end up merging blocks in several steps (resulting in bad complexity). So for example having an option to MergeBlockIntoPredecessor (or a wrapper) to be able to postpone the cleanup to afterwards in such use-cases seems like a good idea to me.

An obstacle here is that code that calls MergeBlockIntoPredecessor tends not to have enough context to pass in a flag to enable RemoveRedundantDbgInstrs at the right time. I can think of two examples:

- SimplifyCFGOpt::simplifyOnce(BB) can do block merging repeatedly. It seems better to call RemoveRedundantDbgInstrs just once in SimplifyCFGOpt::run() after all changes are done.
- CodeGenPrepare::verifyBFIUpdates() can also do block merging repeatedly, but it seems better to call RemoveRedundantDbgInstrs after all the merging is done.

> There actually is a pass that only is used in lit tests (afaik) that is doing this kind of cleanup (RedundantDbgInstElimination). But if we want to cleanup along the way, we might end up wanting to run that pass after every pass that may introduce redundant dbg instrs. In the end that might be more costly, since we wouldn't know if the earlier passes actually did any transformations or not. The currently implementation tries to limit the amount of cleanup by doing it at transforms that are known to introduce redundant dbg instrs.
>
> One thing I realize is that we probaly spend time looking for dbg instrs also in non-debug builds, right? I'm not sure if there is a quick way to check if the IR has dbg instrs or not, to make an early return in RemoveRedundantDbgInstrs if the module/function/basicblock is known to not having any dbg instrs. Any ideas on how to do that?

I don't think there's a quick way to check whether a function/basicblock has debug instructions (e.g. you can always inline dbg intrinsics into a function without a !dbg attachment). We could change the IR to make that kind of lookup fast. Strawman idea: we could add a `NumDebugInstrs` bit to BasicBlock or Function, just like llvm::Value has a `IsUsedByMD` bit, and keep it up-to-date whenever the BasicBlock/Function ilist changes. But I think that idea fails two tests: it's not very general (seems very specific to this problem), and it moves us away from the high-level direction of removing debug insts from the instruction stream.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88928