[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:
> > > > 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. :)
> The idea was that if the unique user of `I` (in the return block) is *not* a PHI, I must dominate the user and therefore it is executed at least as often as the user. If that is true, and I does not read memory (among other things) it is beneficial to move it regardless of the unique predecessor thing. If you wanted to prevent that, wouldn't you need the predecessor check for all instructions, not only the ones that may read, in order to *prevent moving* what I describe above. Asked differently, is `%a` below moved? (It is not right now: https://godbolt.org/z/Y5BxGQ)
> 
> ```
> bb0:
>   %a = add i32 %arg0, 1
>   br i1 %c, label %bb1, label %bb2
> bb1:
>   br label %bb3
> bb2:
>   br label %bb3
> bb3:
>   %p = phi i32 [0, %bb1], [1, %bb2]
>   %r = add i32 %p, %a
>   ret i32 %r
> ```
> I'd say we want the movement but we need a test.
> 
> ---
> 
> We should also have this as a test: https://godbolt.org/z/U8KrtS
> It shows the argument you made earlier, don't sink into more often executed block.
> 
Ok, I will add these tests.


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

https://reviews.llvm.org/D80120