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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert 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:
> 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?


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

https://reviews.llvm.org/D80120