[PATCH] D84951: [LV] Try to sink users recursively for first-order recurrences.

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added a comment.

Sorry, I meant to do a more thorough review before approving. I had a closer look today and found some nits and a couple of questions. Please see inline comments.



================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:706
 
   // Returns true, if all users of I are dominated by DominatedBy.
+  SetVector<Instruction *> UserWorkList;
----------------
nit: this comment is not describing the lambda that follows it.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:756
+  // Try to recursively sink instructions and their users after Previous.
+  for (unsigned I = 0; I < UserWorkList.size(); ++I) {
+    Instruction *Current = UserWorkList[I];
----------------
We need to make sure we don't visit the same user (from the `UserWorkList`) multiple times. For example if we have a hypothetical scenario where:
```
%0 = phi [..., %3], [...]
%1 = phi [...], [Previous] ; The "Phi" that seeds the algorithm.
%2 = add %1, %0
%3 = add %2, 1
<Previous>
...
```
... but I guess that's handled because worklist is actually a set-like container so if we try to add a user that already exists in the worklist, the insertion won't take place and the size of the worklist is unaffected. Could you please put a comment about the choice of the container and how the algorithm guarantees termination?



================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:762
 
-  return allUsesDominatedBy(Phi, Previous);
+  // We can sink all users of Previous. Update the mapping.
+  for (Instruction *I : InstrsToSink)
----------------
users of Previous -> users of Phi


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-complex.ll:287
+
+; Users that are phi nodes cannot be sunk.
+define void @cannot_sink_phi(i32* %ptr) {
----------------
The comment implies that all phi nodes prevent sinking, but I think here the reason `%first_time.1` cannot be sunk past `%for.next` is because (one of) the user(s) of `%first_time.1` is the Previous itself. If that's correct, can we say "some phi nodes"?


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-complex.ll:294
+
+loop.header:                                      ; preds = %if.end128, %for.cond108.preheader
+  %iv = phi i64 [ 1, %entry ], [ %iv.next, %loop.latch ]
----------------
remove or update the comment after `;` (here and on lines 300, 303 and 306)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84951