[PATCH] D48914: [PGOMemOPSize] Preserve the DominatorTree

[Chijun Sima via Phabricator via llvm-commits]

NutshellySima added inline comments.


================
Comment at: lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp:346
 
-  BasicBlock *DefaultBB = SplitBlock(BB, MI);
+  BasicBlock *DefaultBB = SplitBlock(BB, MI, &DT);
   BasicBlock::iterator It(*MI);
----------------
dmgreen wrote:
> 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).
For converting splitBlock to use the incremental updater, because DTU can return the current UpdateStratrgy, I think we can implement the code to use the incremental way when the strategy is lazy and use the current approach + PDT support when the strategy is eager. 

I used this approach in D48967 (llvm::MergeBasicBlockIntoOnlyPred and llvm::MergeBlockIntoPredecessor).

If some timing/benchmarking shows that the performance won't be affected too much, we can then make splitBlock to use the incremental updater under both updateStrategies.


https://reviews.llvm.org/D48914