[PATCH] D154309: [LV] Do not add load to group if it moves across conflicting store.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

This raises some thoughts (post-commit), discussed with @gilr, including the need for a more thorough fix.



================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1090
 // with other accesses that may precede it in program order. Note that a
 // bottom-up order does not imply that WAW dependences should not be checked.
 void InterleavedAccessInfo::analyzeInterleaving(
----------------
Augment the above explanation to address  CompletedLoadGroups?


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1141
         Group = createInterleaveGroup(B, DesB.Stride, DesB.Alignment);
       }
       if (B->mayWriteToMemory())
----------------
Can hoist it further - a newly created group is surely not completed.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1175
       // the boundaries of the (2, 4) group.
       if (!canReorderMemAccessesForInterleavedGroups(&*AI, &*BI)) {
         // If a dependence exists and A is already in a group, we know that A
----------------
Sketch of one option to fix insertion of Group into CompletedLoadGroups whenever/as-soon-as needed.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1188
           StoreGroups.remove(StoreGroup);
           releaseGroup(StoreGroup);
         }
----------------
Note that it may suffice to take `A` out of its StoreGroup, along with all other members that precede `B`, but not necessarily dismantle `StoreGroup` completely. Worth adding some tests.

The case of a store obstructing a store-group seems symmetric to the case of a store obstructing a load-group, if store-groups are collected top-down (separately from continuing to collect load-groups bottom-up), and may then be better formed along with an analogous `CompletedStoreGroups`. WDYT?


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1191
+        // If B is a load and part of an interleave group, no earlier loads can
+        // be added to B's interleave group, because this would mean the load B
+        // would need to be moved across store A. Mark the interleave group as
----------------
"(along with all other loads in B's interleave group)"

Unfortunately, earlier loads may already have been added to B's interleave group at this point. Those could either be filtered out now or prevented from insertion earlier. One way to perform the latter is sketched above.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1194-1195
+        // complete.
+        if (isInterleaved(B) && isa<LoadInst>(B)) {
+          InterleaveGroup<Instruction> *LoadGroup = getInterleaveGroup(B);
+
----------------
We already got the interleave group of B, aka `Group`, and better rename it `GroupB` (along with renaming above `StoreGroup` to `GroupA`)?


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1207
         // either case, we can't add additional instructions to B's group. B
         // will only form a group with instructions that it precedes.
         break;
----------------
"we can't add additional instructions to B's group" - i.e., should mark B's group as completed.


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/interleaved-accesses-hoist-load-across-store.ll:105-106
   %gep.iv.1.plus.2= getelementptr inbounds i32, ptr %arr, i64 %iv.1.plus.2
   %l2 = load i32, ptr %gep.iv.1.plus.2
   %l3 = load i32, ptr %gep.iv.2
   %add = add i32 %l3 , %l2
----------------
Swapping these two loads circumvents the current `CompletedLoadGroups`, and deserves a separate test case.

This is because only the load which creates an interleaved group (the one appearing last in program order) is compared with obstructing stores.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154309