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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
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);
----------------
wenlei wrote:
> 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 ]
> ```
The outer loop already iterates over all uses. The other phi operands will be visited by it later.


================
Comment at: llvm/test/Transforms/LICM/loopsink-phi.ll:48
+bb:
+  %i = tail call i32 @_Z3bari(i32 %arg1), !prof !31
+  %i2 = icmp eq i32 %arg, 0
----------------
This `!prof` metadata probably not needed. In fact, the entire call looks unnecessary, can just be an arg.


================
Comment at: llvm/test/Transforms/LICM/loopsink-phi.ll:142
+!41 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !36, producer: "clang version 8.0.20181009 ", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, nameTableKind: None)
+!42 = !DILocation(line: 6, column: 38, scope: !35)
----------------
Drop dead debug metadata.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152772