[LAA] Fix incorrect dependency classification.

[labrinea]

As shown in #70473, the following loop was not considered safe to vectorize.
When determining the memory access dependencies in a loop which has negative
iteration step, we invert the source and sink of the dependence. Perhaps we
should just invert the operands to getMinusSCEV(). This way the dependency is
not regarded to be true, since the users of the IsWrite variables, which
correspond to each of the memory accesses, rely on program order and therefore
should not be swapped.

void vectorizable_Read_Write(int *A) {
for (unsigned i = 1022; i >= 0; i--)
A[i+1] = A[i] + 1;
}

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Alexandros Lamprineas (labrinea)

Changes

As shown in #70473, the following loop was not considered safe to vectorize. When determining the memory access dependencies in a loop which has negative iteration step, we invert the source and sink of the dependence. Perhaps we should just invert the operands to getMinusSCEV(). This way the dependency is not regarded to be true.

void vectorizable_Read_Write(int *A) {
for (unsigned i = 1022; i >= 0; i--)
A[i+1] = A[i] + 1;
}

---

Full diff: https://github.com/llvm/llvm-project/pull/70819.diff

2 Files Affected:

• (modified) llvm/lib/Analysis/LoopAccessAnalysis.cpp (+2-10)
• (modified) llvm/test/Analysis/LoopAccessAnalysis/forward-negative-step.ll (+2-5)

diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
index 3d1edd5f038a25e..7ab2959c4d88a34 100644
--- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp
+++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
@@ -1885,17 +1885,9 @@ MemoryDepChecker::isDependent(const MemAccessInfo &A, unsigned AIdx,
 
   // If the induction step is negative we have to invert source and sink of the
   // dependence.
-  if (StrideAPtr < 0) {
-    std::swap(APtr, BPtr);
-    std::swap(ATy, BTy);
-    std::swap(Src, Sink);
-    std::swap(AIsWrite, BIsWrite);
-    std::swap(AIdx, BIdx);
-    std::swap(StrideAPtr, StrideBPtr);
-  }
-
   ScalarEvolution &SE = *PSE.getSE();
-  const SCEV *Dist = SE.getMinusSCEV(Sink, Src);
+  const SCEV *Dist = StrideAPtr < 0 ? SE.getMinusSCEV(Src, Sink) :
+                                      SE.getMinusSCEV(Sink, Src);
 
   LLVM_DEBUG(dbgs() << "LAA: Src Scev: " << *Src << "Sink Scev: " << *Sink
                     << "(Induction step: " << StrideAPtr << ")\n");
diff --git a/llvm/test/Analysis/LoopAccessAnalysis/forward-negative-step.ll b/llvm/test/Analysis/LoopAccessAnalysis/forward-negative-step.ll
index 89c1737fb730513..683c44be6da87fc 100644
--- a/llvm/test/Analysis/LoopAccessAnalysis/forward-negative-step.ll
+++ b/llvm/test/Analysis/LoopAccessAnalysis/forward-negative-step.ll
@@ -2,8 +2,6 @@
 
 target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
 
-; FIXME: This should be vectorizable
-
 ; void vectorizable_Read_Write(int *A) {
 ;  for (unsigned i = 1022; i >= 0; i--)
 ;    A[i+1] = A[i] + 1;
@@ -11,10 +9,9 @@ target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
 
 ; CHECK: function 'vectorizable_Read_Write':
 ; CHECK-NEXT:   for.body:
-; CHECK-NEXT:     Report: unsafe dependent memory operations in loop
-; CHECK-NEXT:     Forward loop carried data dependence that prevents store-to-load forwarding.
+; CHECK-NEXT:     Memory dependences are safe
 ; CHECK-NEXT:     Dependences:
-; CHECK-NEXT:       ForwardButPreventsForwarding:
+; CHECK-NEXT:       Forward:
 ; CHECK-NEXT:           %0 = load i32, ptr %arrayidx, align 4 ->
 ; CHECK-NEXT:           store i32 %add, ptr %gep, align 4
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[huntergr-arm]

LGTM, though could you please run git-clang-format over the patch before merging.

[fhahn]

Thanks for the update, there are a few things I'd still like to check, not sure if I will have time today, but should definitely get this done by Monday!

[labrinea]

Thanks for the update, there are a few things I'd still like to check, not sure if I will have time today, but should definitely get this done by Monday!

Ping

[fhahn]

Thanks for the update.

I had a closer look at all uses of the swapped values, and the swapping is only needed for the printing. The other uses all check that they are the same, except for the IsWrite.

But for those all uses expect them in program order, so not swapping them makes sense.

I added another test in 5d35342 that is a variant of. the current test case that shows how we currently also incorrectly indentify a dependence that prevents forwarding as forward only (the commit also auto-generates the checks and cleans up the value names a bit).

Could you rebase and check if the test now also gets handled correctly?

[labrinea]

Thanks for the update.

I had a closer look at all uses of the swapped values, and the swapping is only needed for the printing. The other uses all check that they are the same, except for the IsWrite.

But for those all uses expect them in program order, so not swapping them makes sense.

I added another test in 5d35342 that is a variant of. the current test case that shows how we currently also incorrectly indentify a dependence that prevents forwarding as forward only (the commit also auto-generates the checks and cleans up the value names a bit).

Could you rebase and check if the test now also gets handled correctly?

Thanks Florian, the new test does get handled correctly as far as I understand.

[fhahn]

Thanks for rebasing on top of the test changes, good to see that's fixed now as well! I pushed a commit factoring out the logic to compute the distance, stride & co to a helper function in 9645267. This limits the scope of the swapped variables and makes it easier to see why not swapping them is fine.

Would be good to rebase on top of that again and update the description of the PR with a more precise explanation of why IsWrite should not be swapped: their users rely on program order. Would still be good to retain the original printing order.

After 9645267, retrieving and checking IsWrite can be easily be moved out of the helper, as it is independent of computing the distance/stride.

[fhahn]

Would be good to rebase on top of that again and update the description of the PR with a more precise explanation of why IsWrite should not be swapped: their users rely on program order. Would still be good to retain the original printing order.

Adding to that the the other values don’t neddd to be swapped, as the other uses just check if they are the same.

[labrinea]

IIUC we should still swap source and sink for printing, as otherwise it may be confusing that the difference is not what we expect (Sink - Src)?

Would be good to also add a test for the debug output.

I must have missed this comment. I think we should not be altering the order of Sink and Source when printing them, because it would be as if we claimed that the Sink is the Source and the Source is the Sink, which is not the case. All we are doing is inverting the subtraction operands because one preceeds the other in the memory layout. Having said that @fhahn are you happy with the updated description and rebase?

[fhahn]

IIUC we should still swap source and sink for printing, as otherwise it may be confusing that the difference is not what we expect (Sink - Src)?
Would be good to also add a test for the debug output.

I must have missed this comment. I think we should not be altering the order of Sink and Source when printing them, because it would be as if we claimed that the Sink is the Source and the Source is the Sink, which is not the case. All we are doing is inverting the subtraction operands because one preceeds the other in the memory layout. Having said that @fhahn are you happy with the updated description and rebase?

They are still swapped for computing the distance (even though std::swap isn't used any longer), so for all places that use the distance, they should be considered swapped IIUC. As I mentioned earlier, Distance is supposed to be (Sink - Src). The only place that requires them in program order is the read/write info for couldPreventStoreLoadForward, which probably deserves a comment.

[labrinea]

Ping. I have changed the printing order back to how it was before and I have added a test for it.

[fhahn]

LGTM thanks! Please take a look at the comments for llvm/test/Analysis/LoopAccessAnalysis/print-order.ll before landing.