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

Tue Oct 20 16:33:58 PDT 2020

vsk added a comment.

In D88928#2343046 <https://reviews.llvm.org/D88928#2343046>, @ctetreau wrote:

> I don't know if this patch as-is exhaustively removes redundant debug instructions in all cases it did before, or if we care, so I can't approve it, but I am OK with this approach.
>
> I would like to see this issue addressed sooner than later though. If this patch is merged, then D89818 <https://reviews.llvm.org/D89818> should be abandoned.

@ctetreau thanks for taking a look. This patch changes llvm to remove redundant dbg insts in fewer instances than it did before, but I think that's ok. Looking through the history:

- D71480 <https://reviews.llvm.org/D71480> replaced some hand-rolled duplicated dbg inst removal code with a call to RemoveRedundantDbgInstrs
- The original removal code was added for llvm.org/PR35113 ; I've fixed up LoopRotationUtils.cpp to avoid breaking the associated test

As I mentioned in D89818 <https://reviews.llvm.org/D89818>, I think the RemoveRedundantDbgInstrs behavior should be opt-in as it has a compile-time cost.

I was hoping for another +1 after Jeremy lgtm'd this change to be safe. Seeing as this appears to be time-sensitive, if there aren't any concerns/objections I'll land this by end of day tomorrow (PST).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88928