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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Thanks for following-up on this!



================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1168
       else
         LoadGroups.insert(GroupB);
     }
----------------
nit: while we're here, suffice to place this classification of GroupB and its insertion into either StoreGroups or LoadGroups, next to its creation above.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1194
       // the boundaries of the (2, 4) group.
-      if (!canReorderMemAccessesForInterleavedGroups(&*AI, &*BI)) {
+      //
+      //
----------------
nit: one empty line suffices?

Would it be clearer to do something like:

```
      auto GroupA = getInterleaveGroup(A);
      if (GroupA && StoreGroups.contains(GroupA) &&
          !canReorderMemAccessesForInterleavedGroups(&*AI, &*BI)) {
          LLVM_DEBUG(dbgs() << "LV: Invalidated store group due to "
                               "dependence between " << *A << " and "<< *B << '\n');
        StoreGroups.remove(GroupA);
        releaseGroup(GroupA);
      }
      if (A->mayWriteToMemory() && GroupB && LoadGroups.contains(GroupB)) {
        bool CompleteGroupB = false;
        for (uint32_t Index = 0; Index < GroupB->getFactor(); ++Index) {
          Instruction *MemberOfGroupB = GroupB->getMember(Index);
          if (MemberOfGroupB &&
              !canReorderMemAccessesForInterleavedGroups(
                  &*AI, &*AccessStrideInfo.find(MemberOfGroupB))) {
            CompleteGroupB = true;
            break;
          }
        }
        if (CompleteGroupB) {
          LLVM_DEBUG(dbgs() << "LV: Marking interleave group for " << *B
                            << " as complete.\n");
            CompletedLoadGroups.insert(GroupB);
          break;
        }
      }

```
Would be nicer with an `if (std::any_of(...)) { ...; break; }`, with support from InterleaveGroup to Iterate over its members.

A test that requires both releasing GroupA and completing GroupB would help emphasize that the former must precede the latter, due to its `break`.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1216
+      }
+      if (!DependentInst) {
         // If a dependence exists and A is already in a group, we know that A
----------------
Should be `if (DependentInst)`?


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1248
         // will only form a group with instructions that it precedes.
         break;
       }
----------------
Suffice to place this `break` only when indicating that (load) GroupB is completed; can continue to expand (store) GroupB even if (store) GroupA is released?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155520