[PATCH] D48914: [PGOMemOPSize] Preserve the DominatorTree

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

The updates looks correct to me. Can you add a test for pgo-memop-opt that preserves the DT and verifies it's correct.



================
Comment at: lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp:346
 
-  BasicBlock *DefaultBB = SplitBlock(BB, MI);
+  BasicBlock *DefaultBB = SplitBlock(BB, MI, &DT);
   BasicBlock::iterator It(*MI);
----------------
NutshellySima wrote:
> dmgreen wrote:
> > Why not use DTU for the whole thing ;)
> SplitBlock currently only accepts DT. I'll take care of making these APIs consistent later.
That sounds sensible. To do this bit at a time.

If/when you do try to convert SplitBlock to take a DTU, I think it may be best to just convert it to use the incremental updater, not using what DominatorTreeBase::splitBlock does right now. See D14723 if you want to make PDT's work with splitBlock, but I suspect the incremental updater will easier and not much slower (especially if the updates can be done lazily).


================
Comment at: lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp:435
+  if (DomTreeGetter)
+    DT = &DomTreeGetter->getDomTree();
+  return PGOMemOPSizeOptImpl(F, BFI, ORE, DT);
----------------
This is often done with "DominatorTree *DT = DTWP ? &DTWP->getDomTree() : nullptr;" to put it all on a single line.


https://reviews.llvm.org/D48914