Skip to content

[LAA] Pass maximum stride to isSafeDependenceDistance. #90036

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions llvm/lib/Analysis/LoopAccessAnalysis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1805,20 +1805,20 @@ void MemoryDepChecker::mergeInStatus(VectorizationSafetyStatus S) {
}

/// Given a dependence-distance \p Dist between two
/// memory accesses, that have the same stride whose absolute value is given
/// in \p Stride, and that have the same type size \p TypeByteSize,
/// in a loop whose takenCount is \p BackedgeTakenCount, check if it is
/// possible to prove statically that the dependence distance is larger
/// than the range that the accesses will travel through the execution of
/// the loop. If so, return true; false otherwise. This is useful for
/// example in loops such as the following (PR31098):
/// memory accesses, that have strides in the same direction whose absolute
/// value of the maximum stride is given in \p MaxStride, and that have the same
/// type size \p TypeByteSize, in a loop whose takenCount is \p
/// BackedgeTakenCount, check if it is possible to prove statically that the
/// dependence distance is larger than the range that the accesses will travel
/// through the execution of the loop. If so, return true; false otherwise. This
/// is useful for example in loops such as the following (PR31098):
/// for (i = 0; i < D; ++i) {
/// = out[i];
/// out[i+D] =
/// }
static bool isSafeDependenceDistance(const DataLayout &DL, ScalarEvolution &SE,
const SCEV &BackedgeTakenCount,
const SCEV &Dist, uint64_t Stride,
const SCEV &Dist, uint64_t MaxStride,
uint64_t TypeByteSize) {

// If we can prove that
Expand All @@ -1838,7 +1838,7 @@ static bool isSafeDependenceDistance(const DataLayout &DL, ScalarEvolution &SE,
// will be executed only if LoopCount >= VF, proving distance >= LoopCount
// also guarantees that distance >= VF.
//
const uint64_t ByteStride = Stride * TypeByteSize;
const uint64_t ByteStride = MaxStride * TypeByteSize;
const SCEV *Step = SE.getConstant(BackedgeTakenCount.getType(), ByteStride);
const SCEV *Product = SE.getMulExpr(&BackedgeTakenCount, Step);

Expand Down Expand Up @@ -2046,14 +2046,15 @@ MemoryDepChecker::Dependence::DepType MemoryDepChecker::isDependent(

ScalarEvolution &SE = *PSE.getSE();
auto &DL = InnermostLoop->getHeader()->getModule()->getDataLayout();
uint64_t MaxStride = std::max(StrideA, StrideB);

// If the distance between the acecsses is larger than their absolute stride
// multiplied by the backedge taken count, the accesses are independet, i.e.
// they are far enough appart that accesses won't access the same location
// across all loop ierations.
if (HasSameSize && CommonStride &&
// If the distance between the acecsses is larger than their maximum absolute
// stride multiplied by the backedge taken count, the accesses are independet,
// i.e. they are far enough appart that accesses won't access the same
// location across all loop ierations.
if (HasSameSize &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think StrideA/StrideB are set to 0 if the stride could not be computed (and then indistinguishable from no stride; getPtrStride actually returns an std::optional but it is lost somewhere). I think we must ensure that neither StrideA nor StrideB are zero.

Or even better: Keep passing on the std::optional from getPtrStride.

Or even better: Use StrideA or StrideB depending on whether Dist is negative or positive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey must be non-zero for now which should be ensured here

if (!StrideAPtr || !StrideBPtr || (StrideAPtr > 0 && StrideBPtr < 0) ||

Or even better: Use StrideA or StrideB depending on whether Dist is negative or positive.

That sounds like a good improvement that should help additional cases. Would you prefer this pulled in this patch or separate? In any case, I'll first need to add additional test cases to cover this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks for pointing me there.

isSafeDependenceDistance(DL, SE, *(PSE.getBackedgeTakenCount()), *Dist,
*CommonStride, TypeByteSize))
MaxStride, TypeByteSize))
return Dependence::NoDep;

const SCEVConstant *C = dyn_cast<SCEVConstant>(Dist);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,6 @@ define void @forward_dep_known_safe_due_to_backedge_taken_count(ptr %A) {
; CHECK-NEXT: loop:
; CHECK-NEXT: Memory dependences are safe
; CHECK-NEXT: Dependences:
; CHECK-NEXT: Forward:
; CHECK-NEXT: %l = load i32, ptr %gep.mul.2, align 4 ->
; CHECK-NEXT: store i32 %add, ptr %gep, align 4
; CHECK-EMPTY:
; CHECK-NEXT: Run-time memory checks:
; CHECK-NEXT: Grouped accesses:
; CHECK-EMPTY:
Expand Down Expand Up @@ -80,13 +76,8 @@ exit:
define void @unknown_dep_known_safe_due_to_backedge_taken_count(ptr %A) {
; CHECK-LABEL: 'unknown_dep_known_safe_due_to_backedge_taken_count'
; CHECK-NEXT: loop:
; CHECK-NEXT: Report: unsafe dependent memory operations in loop. Use #pragma clang loop distribute(enable) to allow loop distribution to attempt to isolate the offending operations into a separate loop
; CHECK-NEXT: Unknown data dependence.
; CHECK-NEXT: Memory dependences are safe
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be wrong due to my previous note: If the backedge taken count is not known then there is no safe distance between the pointers (unless the stride is 0, diferent from not computable)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I was confused here why a previously unsafe dep became safe when the title says unknown_dep_known_safe_due_to_backedge_taken_count. I think you specifically added this test case in d5f2753 to fix here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly, the test was specifically added for this PR, although I should probably have added a TODO....

; CHECK-NEXT: Dependences:
; CHECK-NEXT: Unknown:
; CHECK-NEXT: %l = load i32, ptr %gep, align 4 ->
; CHECK-NEXT: store i32 %add, ptr %gep.mul.2, align 4
; CHECK-EMPTY:
; CHECK-NEXT: Run-time memory checks:
; CHECK-NEXT: Grouped accesses:
; CHECK-EMPTY:
Expand Down