[PATCH] D80120: [InstCombine] Sink pure instructions down to return and unreachable blocks

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev marked an inline comment as done.
mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3309
+    if (DestBlock->getUniquePredecessor() != I->getParent())
+      return false;
     for (BasicBlock::iterator Scan = I->getIterator(),
----------------
mkazantsev wrote:
> jdoerfert wrote:
> > mkazantsev wrote:
> > > mkazantsev wrote:
> > > > jdoerfert wrote:
> > > > > I first thought:
> > > > > This is more restrictive than you want it to be.
> > > > > `DestBlock != I->getParent()->getUniqueSuccessor` is what u want.
> > > > > (There is even a less strict restriction but that is more complicated.)
> > > > > That allows `DestBlock` to have multiple predecessors (maybe add at test).
> > > > > 
> > > > > Then I realized this is also the test outside. Though, I still think the above is what we want here.
> > > > This is not true. We don't want to sink from A to C in case:
> > > >   A    B
> > > >    \  /
> > > >     C
> > > > because BC may be a hot path and A is "nearly never executed" block.
> > > Maybe the example above will be more clear like this:
> > > 
> > >     A
> > >     | 
> > >     C<-|
> > >     |  |
> > >     B--|
> > > 
> > > Though, we want to sink from A to either B or C in case
> > >     A
> > >    /  \
> > >   B    C
> > (I guess I should not look at code tired. Since I'm still doing it, please continue to be patient with me.)
> > 
> > The first version of this was not checking the relation, right?
> > 
> > Now, could we check the UserInst to be not a PHI to avoid the problematic cases and allow 
> > ```
> >  [I]
> >  / \
> > [] []
> >  \ /
> >  [ = .. I]
> > ````
> > Do we want that?
> UserInst being a Phi is a special case. General rule for InstCombine is that it does not increase number of instructions (unless it gives some clear benefits). If user in the block below is a phi, we cannot sink `I` without duplication. We are not doing this.
> 
> > Do we want that?
> For loads - we don't, and the only reason of this is that `[]` is a potentially big piece of code that needs a full-scale alias analysis. InstCombine is supposed to be lightweight and it's not doing that.
> 
> For not loads - my patch will handle this (supposed that user block ends with return).
Actually, if `I` is a Phi, and user block does not have other inputs except for those that use `I` - maybe we could sink, but it's going FAAAR beyond the scope of this patch. :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80120/new/

https://reviews.llvm.org/D80120