[PATCH] D48383: [Dominators] Add the DomTreeUpdater class

[Chijun Sima via Phabricator via llvm-commits]

NutshellySima added inline comments.


================
Comment at: include/llvm/IR/DomTreeUpdater.h:142
+  /// It must only be called when it has a DomTree.
+  DominatorTree &flushDomTree();
+
----------------
NutshellySima wrote:
> dmgreen wrote:
> > brzycki wrote:
> > > I'd actually prefer if flushDomTree() and flushPostDomTree(), and flushAll() were private members. Users shouldn't have to think about flushing state. Instead, have all delections and flushes happen when they request access to a DT or PDT reference such as:
> > > 
> > > ```
> > > auto DTU = DomTreeUpdater(DT, PDT, Lazy);
> > > ...
> > > DTU->applyUpdates({Foo, Bar, Insert});
> > > DTU->applyUpdates({Bar, Baz, Delete});
> > > ...
> > > auto DT = DTU->getDT(); // flush() and delete BBs happen here automatically.
> > > ```
> > What do you think of having DTU automatically flush remaining updates in it's deconstructor. So that things like these (to steal your examples) work:
> >   void F(..)
> >   {
> >     auto DTU = DomTreeUpdater(DT, PDT, Lazy);
> >     ...
> >     DTU->applyUpdates({Foo, Bar, Insert});
> >     DTU->applyUpdates({Bar, Baz, Delete});
> >   }
> > 
> > or
> > 
> >   {
> >     auto DTU = DomTreeUpdater(DT, PDT, Lazy);
> >     SomeFunctionThatUpdatesTrees(..., DTU, ...);
> >   }
> I think flushAll() can't be private members:
> 
> ```
> auto DTU = DomTreeUpdater(DT, Lazy);
> ... // Make BB0 forward/backward-unreachable.
> DTU->applyUpdates(SomeUpdates);
> auto DT = DTU->getDT();
> ...
> // Now delete BB0
> DTU->deleteBB(BB0);
> // Now there is no pendingDTUpdates and can only use flushAll() to get BB0 removed from its parent and deleted.
> ```
More information on the above comments:
I remember some code will break if the internal logic of deleteBB calls removeFromParent() and deletes BB immediately when there isn't any pending DT/PDT updates.
So, I think it is needed to leave a flushAll() function to help user flush BB awaiting deletion.


https://reviews.llvm.org/D48383