[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(),
----------------
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).


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

https://reviews.llvm.org/D80120