[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 ?");
----------------
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. 


https://reviews.llvm.org/D26954