[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:748
+  // TODO: Consider extending this sinking to handle memory instructions.
+  SetVector<Instruction *> UserWorkList;
+  auto PushUsers = [&UserWorkList](Instruction *I) {
----------------
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.


================
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))
----------------
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.


================
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);
----------------
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?


================
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) {
----------------
Ayal wrote:
> bmahjour wrote:
> > 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"?
> The (first) reason `%first_time.1` cannot be sunk is because it appears outside the header and is not dominated by Previous. The fact that it feeds Previous is a second sinking-preventing reason.
Good point, I adjusted the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84951