[PATCH] D155520: [LV] Complete load groups and release store groups in presence of dependency
Ayal Zaks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 19 04:26:07 PDT 2023
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
More information about the llvm-commits
mailing list