[PATCH] D26954: Remove unnecessary IDom check

[Xin Tong via Phabricator via llvm-commits]

trentxintong added inline comments.


================
Comment at: lib/Transforms/Scalar/Sink.cpp:174
+    // tree.
+    assert((*I)->getIDom()->getBlock() == Inst->getParent() &&
+           "Broken dominator tree ?");
----------------
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.


https://reviews.llvm.org/D26954