[PATCH] D74642: [CodeGenPrepare] Speed up placeDbgValues, NFC

Fri Feb 14 15:16:08 PST 2020

rnk added inline comments.

================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:7273
+    // from 10 minutes to 50 milliseconds. We can't just use OrderedBasicBlock
+    // because moving around dbg.values causes invalidation.
+    SmallPtrSet<const Instruction *, 32> VisitedInsts;
----------------
vsk wrote:
> rnk wrote:
> > I guess by the same reasoning this is still useful even if we had D51664. Although, if no reordering is required because all defs are before uses, D51664 would still help, and I think that is the common case. I assume the webkit case is a long BB of:
> >   def A ; dbgval(A) ; def B ; dbgval(B)...
> I believe D51664 makes this patch unnecessary. I grabbed the NumDbgValueMoved statistic from the WebKit test case: it was 0. This patch might still improve performance if CGP were to encounter lots of weird dbg.value use-before-def. But I think we want to fix those bugs earlier in the pipeline anyway.
> 
> Should we just hold off on landing this, or put it in as a stopgap until D51664 is merged (and file a PR for it to be reverted afterwards)?
I guess in the long run it would be best to not have this complexity here. I think the numbering invalidation logic could use whatever MLIR does to avoid invalidating the numbering too often, and then this code can remain simple.

So, I agree we should either land this as a stop gap with a plan to revert, or wait. I plan to leave the RFC alone until next Thursday, and hopefully land D51664 then. Given that timeline, you can either land or wait.

I suspect this will not be the only code in LLVM that can be simplified after the instruction numbering lands.

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

https://reviews.llvm.org/D74642