[PATCH] D26954: Remove unnecessary IDom check

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added inline comments.


================
Comment at: lib/Transforms/Scalar/Sink.cpp:174
+    // tree.
+    assert((*I)->getIDom()->getBlock() == Inst->getParent() &&
+           "Broken dominator tree ?");
----------------
trentxintong wrote:
> trentxintong wrote:
> > dberlin wrote:
> > > I wouldn't even bother with this assert.
> > > Just remove it and commit.
> > > 
> > > 
> > > Honestly, this whole code is silly.
> > > 
> > > If you want to take a whack at it,  the proper (IMHO) way to do this is:
> > > 
> > > ncdblock  = find nearestcommondominator(blocks that contain uses)
> > > 
> > > If ncdblock == Inst->getParent(), stop, can't sink.
> > > Otherwise, 
> > > while (!AcceptableTarget(Inst, ncdblock) && ncdblock != Inst->getParent())
> > > ,    NCDBlock = getIDom(NCDBlock);
> > > 
> > > This will sink as low as possible.
> > > You can then remove the allusesdominatedbyblock check from IsAcceptableTarget. Any idom of the ncd must, by definition, still dominate the uses.
> > > Note that when computing NearestCommonDominator(uses), phi uses have to be considered to live at the end of the incoming block they come from to get optimal results.
> > > 
> > Thats what I have in mind as well. However, this would work well for sinking SSA values, but not memory reading/writing instructions.
> > 
> > Sink.cpp has some simple abilities to sink loads and stores. 
> By SSA values I meant instructions without reading/writing to memory.
If we cared, we'd just run MemorySSA, and for loads/stores, and then you have the SSA form you need to sink stores


https://reviews.llvm.org/D26954