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

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/interleaved-accesses-sink-store-across-load.ll:8
 ; that store into the last store (by creating an interleaved store group). This
-; means the loaded %l2 has incorrect value.
-; We do not release this store group correctly because the next interleave group
-; chosen compares only the memory access of last load in program (%l3) against the dependent store location
-; (%gep.iv.1.plus.2) and they are different, thereby incorrectly assuming no
-; dependency. We need to compare against all loads in that interleaved group
-; (%l2 is part of it).
+; means the loaded %l2 will have incorrect value.
 define void @avoid_sinking_store_across_load(ptr %arr) {
----------------
anna wrote:
> Ayal wrote:
> > This additional/distinct testcase of preventing the sinking of a store is fixed by the same patch that compares all member of a load-groupB with a storeA, right?
> Unfortunately, no. That only marks `GroupB` for completion, thereby preventing hoisting of a load to an earlier load group. 
> 
> To show what happens in this test case: we reverse traverse the memory access. So, the first interleave group created is:
> ```
> store i32 %add, ptr %gep.iv.2 <-- first access
> store i32 %mul, ptr %gep.iv.1.plus.2 <-- added into the (store) groupB. 
> ```
> Let's call this StoreGroup1.
> 
> We next come to B: `%l3 = load i32, ptr %gep.iv.2`
> Interleave group created with that inst.
> We create next interleave group:
> ```
> %l3 = load i32, ptr %gep.iv.2
> %l2 = load i32, ptr %gep.iv.1.plus.2
> ```
> Call this `LoadGroup1`.
> 
> Wit that same `B`, we continue `A` accesses with A as `store i32 %mul, ptr %gep.iv.1.plus.2`.
> 
> With the patch I have we will do two things:
> 1. release `StoreGroup1` because we see the store has a dependency with `%l2`
> 2. Mark `LoadGroup1` as complete (and break from this inner loop traversing `AI`).
> 
> If we were to *only* compare `BI` when deciding for store release:
> ```
> if (GroupA && StoreGroups.contains(GroupA) &&
>     !canReorderMemAccessesForInterleavedGroups(&*AI, &*BI))
> ```
> then, we will not release StoreGroup1 since BI (%l3) has no dependency. 
> 
> Which means even though the load group is correctly completed (and we will not hoist %l2 to location of %l1), we will sink the store (store i32 %mul, ptr %gep.iv.1.plus.2) below the dependent load (%l2). 
> 
@Ayal For completeness, here's the diff showing why the fix suggested in the comments won't be enough: https://reviews.llvm.org/differential/diff/542078/. The test in `interleaved-accesses-sink-store-across-load.ll` shows that we still sunk the store incorrectly (see the masked.store at the end). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155520