[PATCH] D152772: [LoopSink] Allow sinking to PHI-use

[Wenlei He via Phabricator via llvm-commits]

wenlei marked 3 inline comments as done.
wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopSink.cpp:182
     // We cannot sink I if it has uses outside of the loop.
     if (!L.contains(LI.getLoopFor(UI->getParent())))
       return false;
----------------
nikic wrote:
> Might it make sense to move the phi node handling before this check? It should be fine to sink an instruction that is used outside the loop into the corresponding exiting block (i.e. the predecessor of the lcssa phi node), and this may be profitable if the exiting block is cold.
I was thinking about that too. But with some digging I found this was considered in the original loop sink patch (D22778) and then was decided to keep it strictly a loop sink pass, instead of general sinking. Perhaps makes sense to keep it that way. 

Original comment/reply:

> Isn't it still okay to try to sink in outside the loop if the user block is cold enough?

> That would become general purpose sinking instead of loop-sinking. And we need to handle alias/invariant differently.


================
Comment at: llvm/lib/Transforms/Scalar/LoopSink.cpp:194
+    BasicBlock *PhiBB = nullptr;
+    for (unsigned i = 0, e = PN->getNumIncomingValues(); i != e; ++i) {
+      Value *PhiVal = PN->getIncomingValue(i);
----------------
nikic wrote:
> You don't need this loop, use `PN->getIncomingBlock(U)` instead.
We need to find all incoming blocks of a value, but `PN->getIncomingBlock(U)` only return one (first) instance? The loop doesn't "break" when a value is found. 

E.g. for value %326, we need to find the 4 incoming blocks for all 4 uses in the PHI. 

```
%343         = phi i64 [ %326, %338 ], [ %291, %305 ], [ %326, %332 ], [ %326, %334 ], [ %326, %337 ]
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152772