[PATCH] D155520: [LV] Complete load groups and release store groups in presence of dependency

[Ayal Zaks via Phabricator via llvm-commits]

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

In D155520#4532840 <https://reviews.llvm.org/D155520#4532840>, @anna wrote:

> Hi Ayal, any more comments? Thanks.

This is fine, thanks Anna for following-up!
Added some minor nits.



================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1205
+        Instruction *DependentInst = nullptr;
+        // If GroupB is a load group, We have to compare AI against all
+        // members of GroupB because if any load within GroupB has a dependency
----------------



================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1207
+        // members of GroupB because if any load within GroupB has a dependency
+        // against AI, we need to mark GroupB as complete and also release the
+        // storeGroup (if A belongs to one). The former prevents incorrect
----------------



================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1208
+        // against AI, we need to mark GroupB as complete and also release the
+        // storeGroup (if A belongs to one). The former prevents incorrect
+        // hoisting of load B above store A while the latter prevents incorrect
----------------



================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1214
+        else if (!canReorderMemAccessesForInterleavedGroups(&*AI, &*BI))
+          DependentInst = B;
+        if (DependentInst) {
----------------
nit: an empty line would help separate the setting of DependentInst from it use.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1218
+          // A has a store dependence on B (or on some load within GroupB) and
+          // is part of a storeGroup. Release A's group to prevent illegalt
+          // sinking of A below B. A will then be free to form another group
----------------



================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1230
+          // can be added to B's interleave group, because this would mean the
+          // DependentInst would need to be moved across store A. Mark the
+          // interleave group as complete.
----------------



================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1167
         // Skip B if no new instructions can be added to its load group.
         continue;
       }
----------------
anna wrote:
> Ayal wrote:
> > Ahh, when B belongs to a completed load group, no A can be added to it, but **early-continuing here** also prevents releasing the store group of any conflicting A. As outlined above: "Even if we don't create a group for B, we continue with the bottom-up algorithm to ensure we don't break any of B's dependences" - continuing the bottom-up algorithm even if the group of B is complete would fix avoid_sinking_store_across_load: considering %l2 as B would release the store group. But other cases may evade this "avoid early-continue here" fix - when the conflict to release a store group is encountered before the store group is formed; here's an example, worth adding to the documentation:
> > 
> > ```
> >       // For example, assume we have the sequence of accesses shown below in a
> >       // stride-3 loop:
> >       //
> >       // (1, 3) is a group | A[i] = a;   // (1)
> >       //                     b = A[i+1]; // (2) |
> >       //                   | A[i+2] = c; // (3)
> >       //                     d = A[i];   // (4) | (2, 4) is a group
> >       // but cannot have both (1,3) and (2,4) groups!
> >       // Because the former sinks (1) to (3), the latter hoists (4) to (2),
> >       // and there's a dependence between (1) and (4).
> >       // Whenever B is a member of a load group, consider it along with
> >       // all its members, because they will all be hoisted to B, or earlier.
> > ```
> > 
> > Note: exact full overlap of load-after-store and store-after-store dependencies, as shown in these examples, are best optimized by eliminating the redundant load or first store, respectively. Partial overlap dependencies may be more involved to resolve, by converting to fixed-order-recurrences and/or hoisting a load above a conditional store.
> > 
> > Note: this algorithm of building interleave groups is worth revisiting, possibly by actually moving group members using VPlan, to both simplify and potentially optimize its decisions, e.g.: should a load group be built or a store group, if either is possible but not both? Keeping a conflicting member out of a group (thereby allowing several others to join) may be better than Completing it.
> > But other cases may evade this "avoid early-continue here" fix - when the conflict to release a store group is encountered before the store group is formed;
> Yes, this is the exact case I faced for the last miscompile (which I haven't fixed yet). The store group wasn't formed yet and your added example for the doc is exactly what happens. 
> Note that when we switch around (3) and (4) in the example, we will not have a miscompile since we formed an interleave store group first (1,3). And then we come to load with B  as `d = A[i]` and identify that the store group (1,3) must be released (since we will traverse backwards and reach A as ` A[i] = a` which is a dependency with B and is part of a store group).
>  My idea to fix that was to record such stores and state they are the "last insertion point" (i.e. other stores can be sunk to the store, but we cannot sink store further down).
> 
> Also, thanks for catching  the "early continue" here. 
> 
> >  here's an example, worth adding to the documentation:
> I will add this documentation when I precommit the miscompile test, if that's okay with you (since it would show the IR example as well)? We will need a separate fix for it as well.
>> here's an example, worth adding to the documentation:
>
> I will add this documentation when I precommit the miscompile test, if that's okay with you (since it would show the IR example as well)? We will need a separate fix for it as well.

Sure, by all means.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155520