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

[Florian Hahn via Phabricator via llvm-commits]

fhahn 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;
----------------
Ayal wrote:
> 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.
This is needed so we conveniently check if arbitrary instructions are in the set, but those instructions should never be inserted. But I can change it to only query the set for instructions in the same BB. (comesBefore already asserts that they are in the same BB)


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:777
+      if (DT->dominates(Previous, UserI)) // We already are good w/o sinking.
+        continue;
 
----------------
Ayal wrote:
> 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)
At this point we don't know if the PHI is in the same block or not. (checked only below). We may incorrectly skip a PHI node in a different BB, which is not dominated by Previous (tested in `cannot_sink_phi` in `first-order-recurrence-complex.ll`). I left the check at it's original place, so it only takes place after we ensure that the parents match.


================
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() ||
----------------
Ayal wrote:
> // We cannot [sink] user.
removed the whole comment.


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


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:783
+          UserI->getParent()->getTerminator() == UserI ||
+          SinkAfter.find(UserI) != SinkAfter.end())
+        return false;
----------------
Ayal wrote:
> Would be good to retain the TODO comment about supporting sinking a user after multiple (deepest) previouses.
Sounds good. I split off the `SinkAfter` check again, so it's clear which single check the comment refers to.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:789
+      if (isa<PHINode>(UserI)) {
+        assert(UserI->getParent() == PhiBB && "expected header PHI");
+        continue;
----------------
Ayal wrote:
> 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.
As mentioned above, I think the check needs to happen after we checked that they are in the same BB. I added the assert to catch inconsistencies if the code is moved, but it's not too helpful. I removed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84951