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

Johannes Doerfert via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue May 19 07:34:06 PDT 2020


jdoerfert added a comment.

If the two tests I mentioned are added and work as expected, I think this is OK. I'm a little worried we move in one case where we shouldn't without the PHI user logic I describe below.



================
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:
> > > 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.



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

https://reviews.llvm.org/D80120





More information about the llvm-commits mailing list