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

[Nikita Popov via Phabricator via llvm-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

Looks reasonable conceptually. Could be extended with critical edge splitting, but not sure how profitable that would be.



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


================
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);
----------------
You don't need this loop, use `PN->getIncomingBlock(U)` instead.


================
Comment at: llvm/test/Transforms/LICM/loopsink-phi.ll:1
+; RUN: opt -S -verify-memoryssa -aa-pipeline=basic-aa -passes=loop-sink < %s | FileCheck %s
+; Make sure that unprofitable loop ICM can be undone by loop sink, and loop sink can handle
----------------
aa-pipeline unnecessary


================
Comment at: llvm/test/Transforms/LICM/loopsink-phi.ll:9
+; CHECK:       .l.cold2
+; CHECK-NEXT:    {{.*}} = add nsw i32 {{.*}}, {{.*}}
+
----------------
Use update_test_checks.py.


================
Comment at: llvm/test/Transforms/LICM/loopsink-phi.ll:11
+
+define dso_local i32 @_Z3fooii(i32 %0, i32 %1) local_unnamed_addr #0 !dbg !30 !prof !36 {
+  %3 = tail call i32 @_Z3bari(i32 %1), !dbg !37, !prof !38
----------------
Remove `!dbg` metadata and name instructions in test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152772