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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:756
+  auto CompareByComesBefore = [](const Instruction *A, const Instruction *B) {
+    if (A->getParent() != B->getParent())
+      return true;
----------------
Assert they have a common parent (as done by comesBefore())?
If sorting should work (somehow) across basic blocks, better enumerate them somehow, otherwise both A < B and A > B may both hold.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:768
+      auto *UserI = cast<Instruction>(User);
+      // Already sunk UserI.
+      if (InstrsToSink.find(UserI) != InstrsToSink.end())
----------------
Indeed, better check this first, than check last if insertion fails!


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:777
+      if (DT->dominates(Previous, UserI)) // We already are good w/o sinking.
+        continue;
 
----------------
The isa<PHInode> case belongs here, with the "we're fine with UserI's current position" case.
(The property we're really after here is //conditional dominance//: Previous dominates UserI conditional on having reached FOR Phi)


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:779
 
-    // Do not try to sink an instruction multiple times (if multiple operands
-    // are first order recurrences).
-    // TODO: We can support this case, by sinking the instruction after the
-    // 'deepest' previous instruction.
-    if (SinkAfter.find(I) != SinkAfter.end())
-      return false;
+      // We cannot user.
+      if (UserI->getParent() != PhiBB || UserI->mayHaveSideEffects() ||
----------------
// We cannot [sink] user.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:782
+          UserI->mayReadFromMemory() ||
+          UserI->getParent()->getTerminator() == UserI ||
+          SinkAfter.find(UserI) != SinkAfter.end())
----------------
While we're here, simply check `!UserI->isTerminator()`?
Worth retaining the uninformative comment?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:783
+          UserI->getParent()->getTerminator() == UserI ||
+          SinkAfter.find(UserI) != SinkAfter.end())
+        return false;
----------------
Would be good to retain the TODO comment about supporting sinking a user after multiple (deepest) previouses.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:789
+      if (isa<PHINode>(UserI)) {
+        assert(UserI->getParent() == PhiBB && "expected header PHI");
+        continue;
----------------
Relevant if isa<PHINode> is moved above.
If it stays here, it's redundant, as we bailed-out earlier if UserI is not in PhiBB.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:804
+    Instruction *Current = WorkList.pop_back_val();
+    if (!TryToPushSinkableUsers(Current))
+      return false;
----------------
Suffice to provide each User of Current to TryToPushSinkableUser[s](), i.e., how about doing

```
    for (User *User : Current->users())
      if (!TryToPushSinkableUser(cast<Instruction>(User)))
        return false;
```
to simplify the lambda?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:748
+  // TODO: Consider extending this sinking to handle memory instructions.
+  SetVector<Instruction *> UserWorkList;
+  auto PushUsers = [&UserWorkList](Instruction *I) {
----------------
fhahn wrote:
> Ayal wrote:
> > The candidates need to be maintained in a **set** in order to avoid considering them repeatedly. May as well have this set ordered by dominance relation, instead of sorting it at the end.
> > E.g., let UserWorkList be a SmallVector that is pushed and popped until empty, where candidates are checked to be sunkable when being pushed into it (instead of when being popped/traversed), after successfully inserting them into the sorted InstrsToSink set. WDYT?
> I updated the code to use a ordered set for InstrToSink as suggested. I think the only thing is that we might check the same instruction multiple times, if it is already dominated by `Previous`. In that case, it won't get inserted into `InstrToSink`. But I think that should be fine.
Agreed. We could cache/memoize a set of confirmed users in addition to InstrToSink, but their re-confirmation should be quick, and reaching users multiple times is probably infrequent.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:778
+    // If we reach a PHI node that is not dominated by Previous, we reached a
+    // header PHI. No need for sinking.
+    if (isa<PHINode>(I))
----------------
fhahn wrote:
> Ayal wrote:
> > This sounds right, given that we only consider I's that post-dominate the FOR phi.
> > Seems somewhat odd though, calls for an assert and a testcase.
> I think there's already a test case where this can happen: `@sink_into_replication_region`. I added an assert that the instruction must be in the same BB as the starting phi.
Agreed. (Nice test, where an add accumulating the FOR phi is sunk after a previous udiv, and the fact that the add feeds its reduction header phi is fine.)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9136
   // Apply Sink-After legal constraints.
-  for (auto &Entry : SinkAfter) {
+  for (auto &Entry : reverse(SinkAfter)) {
     VPRecipeBase *Sink = RecipeBuilder.getRecipe(Entry.first);
----------------
fhahn wrote:
> Ayal wrote:
> > Ayal wrote:
> > > Sort multiple users in reverse instead of reversing all here? 
> > > Or rather - have them sink after each other instead of all sinking after Previous, thereby expressing their desired order and dependencies directly?
> > (BTW, note that by having multiple users sink after each other, it is also possible to reorder them lazily instead of sorting: when moving Sink after Target, check if Target should also be sunk - and if so sink it first, recursively, while taking care to sink each SinkAfter instruction once. Multiple users is the only exception to sinking an instruction after a Target that should itself sink.)
> I went with `Or rather - have them sink after each other instead of all sinking after Previous, thereby expressing their desired order and dependencies directly`
> 
> The dependencies are chained together in `SinkAfter` now. But keeping them sorted to start with seems simpler overall and ensures that map is accurate. WDYT?
Agreed. The map itself is accurate whether sorted or not in this case, but lazy reordering may potentially enter an infinite loop (if buggy), and requires marking sunk instructions. Given that a set is needed to ensure uniqueness, it may as well provide the desired order, as done above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84951