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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

This looks good to me, thanks!
Title might emphasize "[LV] Try to sink **multiple** users ..."
Noting another couple of TODO thoughts.



================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:756
+  auto CompareByComesBefore = [](const Instruction *A, const Instruction *B) {
+    if (A->getParent() != B->getParent())
+      return true;
----------------
fhahn wrote:
> 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)
Ah, ok.
Another potential TODO is to sink users in non-header basic blocks that post-dominate the header, which may extend CompareByComesBefore to distinct blocks using their domination.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:777
+      if (DT->dominates(Previous, UserI)) // We already are good w/o sinking.
+        continue;
 
----------------
fhahn wrote:
> 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.
Ah, right; cannot_sink_phi may be one case where Previous needs to be //hoisted before// a user, instead of vice versa, assuming no other reorder-preventing reasons exist...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84951