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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

In D84951#2777544 <https://reviews.llvm.org/D84951#2777544>, @fhahn wrote:

> In D84951#2776110 <https://reviews.llvm.org/D84951#2776110>, @Ayal wrote:
>
>> The instructions to sink reside inside the header block in the desired order. It should be possible to traverse them, once, from the FOR phi to the terminator or Previous, w/o sorting nor changing order, by maintaining the set of phi's (direct and indirect) users. When encountering such a user during the traversal, check if it is sunkable, append it to be moved after the last sunken user (or after Previous if this is the first user being sunk), and add its users to the set - those that belong to the header block, others need to be dominated by (and distinct from) Previous.
>> Perhaps this would also help support sinking memory instructions in the future.
>> Sounds reasonable?
>
> IIUC your suggestion would require to iterate over the instructions in the header block compared to just traversing the def-use chains from the PHI we are analyzing as we do at the moment? That would work, as we would visit the candidates in the correct order. But requiring to potentially iterate over all instructions in the header block might be worse in practice than sorting the (probably few) instructions that require sinking?
>
> It will be indeed required once we want to sink memory operations, but until then it seems to me that just traversing the def-use chains + sorting would probably be less work in general for the cases we currently support (also given the interface that requires us to analyze each FOR separately). Please let me know if I understood correctly. Happy to update the patch as suggested if you think it's beneficial on its own.

Happy to go with your sorting InstrsToSink approach, added some comments to it below.

Right, this order-preserving alternative requires iterating over the instructions in the header block from the FOR phi until its last header-block-user is encountered. Agree that this would probably take longer than sorting InstrsToSink, e.g., if there's a user close to the end of the header. Mainly pointed out as an alternative, towards supporting sinking memory instructions (supporting multiple FOR phi's would be possible in a single scan, but seems an overkill), and as a response to

> I'm not sure if it is possible to maintain the order by dominance in the map as we go along...

;-)



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


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


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84951