[DA] Dependence analysis does not handle array accesses of different sizes #116630

sebpop · 2024-11-18T14:26:20Z

This fixes bug #16183 where the elements of a same array are accesses as i32 and i64. This is not handled by the array dependence analysis.

llvmbot · 2024-11-18T14:26:58Z

@llvm/pr-subscribers-testing-tools

@llvm/pr-subscribers-llvm-analysis

Author: Sebastian Pop (sebpop)

Changes

This fixes bug #16183 where the elements of a same array are accesses as i32 and i64. This is not handled by the array dependence analysis.

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

2 Files Affected:

(modified) llvm/lib/Analysis/DependenceAnalysis.cpp (+7-1)
(added) llvm/test/Analysis/DependenceAnalysis/DifferentAccessSize.ll (+15)

diff --git a/llvm/lib/Analysis/DependenceAnalysis.cpp b/llvm/lib/Analysis/DependenceAnalysis.cpp
index a4a98ea0bae146..c1028effcfc1bc 100644
--- a/llvm/lib/Analysis/DependenceAnalysis.cpp
+++ b/llvm/lib/Analysis/DependenceAnalysis.cpp
@@ -730,8 +730,14 @@ static AliasResult underlyingObjectsAlias(AAResults *AA,
   const Value *BObj = getUnderlyingObject(LocB.Ptr);
 
   // If the underlying objects are the same, they must alias
-  if (AObj == BObj)
+  if (AObj == BObj) {
+    // The dependence test gets confused if the size of the memory accesses
+    // differ.
+    if (LocA.Size != LocB.Size)
+      return AliasResult::MayAlias;
+
     return AliasResult::MustAlias;
+  }
 
   // We may have hit the recursion limit for underlying objects, or have
   // underlying objects where we don't know they will alias.
diff --git a/llvm/test/Analysis/DependenceAnalysis/DifferentAccessSize.ll b/llvm/test/Analysis/DependenceAnalysis/DifferentAccessSize.ll
new file mode 100644
index 00000000000000..4d5da005ea437b
--- /dev/null
+++ b/llvm/test/Analysis/DependenceAnalysis/DifferentAccessSize.ll
@@ -0,0 +1,15 @@
+; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa 2>&1 \
+; RUN: | FileCheck %s
+
+; The dependence test does not handle array accesses of different sizes: i32 and i64.
+; Bug 16183 - https://github.com/llvm/llvm-project/issues/16183
+; CHECK-LABEL: bug16183_alias
+; CHECK: da analyze - confused!
+
+define i64 @bug16183_alias(i32* nocapture %A) {
+entry:
+  %arrayidx = getelementptr inbounds i32, ptr %A, i64 1
+  store i32 2, ptr %arrayidx, align 4
+  %0 = load i64, ptr %A, align 8
+  ret i64 %0
+}

nikic

This looks like something that should be checked at a higher level than underlyingObjectsAlias, because the access size is not relevant to determine whether the underlying objects alias.

sebpop · 2025-01-07T14:44:49Z

I believe I addressed all the comments.
@nikic could you please have a look again and approve?
Thank you.

nikic

Looking a bit more closely, I'm not sure that "different access sizes" is really the core issue here.

For example, consider this variant of your test case:

define i32 @bug16183_alias(ptr nocapture %A) {
entry:
  %arrayidx = getelementptr inbounds i8, ptr %A, i64 2
  store i32 2, ptr %arrayidx, align 1 
  %0 = load i32, ptr %A, align 1
  ret i32 %0
}

This has equal access sizes, but the two accesses still overlap, because the GEP is only by half the access width. It currently reports da analyze - none! and wouldn't be fixed by your patch.

Without looking at the implementation, I expect the issue here is that the code does not verify that the access sizes and the access stride are compatible.

nikic · 2025-01-07T14:51:43Z

llvm/lib/Analysis/DependenceAnalysis.cpp

-  Value *SrcPtr = getLoadStorePointerOperand(Src);
-  Value *DstPtr = getLoadStorePointerOperand(Dst);
+  const MemoryLocation &LocA = MemoryLocation::get(Dst);
+  const MemoryLocation &LocB = MemoryLocation::get(Src);


LocA, LocB -> LocSrc, LocDst or SrcLoc, DstLoc maybe?

nikic · 2025-01-07T14:51:53Z

llvm/lib/Analysis/DependenceAnalysis.cpp

@@ -3632,6 +3635,8 @@ DependenceInfo::depends(Instruction *Src, Instruction *Dst,

  unsigned Pairs = 1;
  SmallVector<Subscript, 2> Pair(Pairs);
+  Value *SrcPtr = getLoadStorePointerOperand(Src);
+  Value *DstPtr = getLoadStorePointerOperand(Dst);


Can you use const Value *SrcPtr = LocA.Value; here?

No, I can't use const here: SE->getSCEV takes a Value*:

const llvm::SCEV* llvm::ScalarEvolution::getSCEV(llvm::Value*)

nikic · 2025-01-07T14:52:10Z

llvm/test/Analysis/DependenceAnalysis/DifferentAccessSize.ll

+; CHECK-LABEL: bug16183_alias
+; CHECK: da analyze - confused!
+
+define i64 @bug16183_alias(i32* nocapture %A) {


Suggested change

define i64 @bug16183_alias(i32* nocapture %A) {

define i64 @bug16183_alias(ptr nocapture %A) {

github-actions · 2025-01-18T01:58:50Z

✅ With the latest revision this PR passed the undef deprecator.

sebpop · 2025-01-18T02:03:40Z

Looking a bit more closely, I'm not sure that "different access sizes" is really the core issue here.

[...] the code does not verify that the access sizes and the access stride are compatible.

This is more difficult to solve. Please see separate patch #123436 that describes the issue. DA is a symbolic solver that needs to collect a set of assumptions under which the DA produces correct results. We need to implement something similar to what Polly does in collecting a set of symbolic constraints, and those get generated as a runtime test versioning the loops generated by Polly.

@kasuga-fj

This is a work in progress patch to enable loop-interchange by default and is a continuation of the RFC: https://discourse.llvm.org/t/enabling-loop-interchange/82589 Basically, we promised to fix any compile-time and correctness issues in the different components involved here (loop-interchange and dependence analaysis.) before discussing enabling interchange by default. We think are close to complete this; I would like to explain where we are and wanted to check if there are any thoughts or concerns. A quick overview of the correctness and compile-time improvements that we have made include: Correctness: - [LoopInterchange] Remove 'S' Scalar Dependencies (llvm#119345) - [LoopInterchange] Fix overflow in cost calculation (llvm#111807) - [LoopInterchange] Handle LE and GE correctly (PR llvm#124901) @kasuga-fj - [DA] disambiguate evolution of base addresses (llvm#116628) Compile-times: - [LoopInterchange] Constrain number of load/stores in a loop (llvm#118973) - [LoopInterchange] Bail out early if minimum loop nest is not met (llvm#115128) - [LoopInterchange] Hoist isComputableLoopNest() in the control flow (llvm#124247) And in terms of remaining work, we think we are very close to fixing these depenence analysis issues: - [DA] do not handle array accesses of different offsets (llvm#123436) - [DA] Dependence analysis does not handle array accesses of different sizes (llvm#116630) - [DA] use NSW arithmetic llvm#116632 The compile-time increase with a geomean increase of 0.19% looks good (after committing llvm#124247), I think: stage1-O3: Benchmark kimwitu++ +0.10% sqlite3 +0.14% consumer-typeset +0.07% Bullet +0.06% tramp3d-v4 +0.21% mafft +0.39% ClamAVi +0.06% lencod +0.61% SPASS +0.17% 7zip +0.08% geomean +0.19% See also: http://llvm-compile-time-tracker.com/compare.php?from=19a7fe03b4f58c4f73ea91d5e63bc4c6e61f987b&to=b24f1367d68ee675ea93ecda4939208c6b68ae4b&stat=instructions%3Au We might want to look into lencod to see if we can improve more, but not sure it is strictly necessary.

…sizes This fixes bug llvm#16183 where the elements of a same array are accesses as i32 and i64. This is not handled by the array dependence analysis.

sebpop · 2025-01-31T14:10:31Z

@nikic can you check if we can commit this patch independently of #123436

Thanks!

sebpop · 2025-02-20T17:13:26Z

Hi @nikic,
#123436 fixes the test you mentioned above in #116630 (review)
Both #123436 and this PR #116630 are ready for review.

kasuga-fj · 2025-04-11T15:13:21Z

llvm/lib/Analysis/DependenceAnalysis.cpp

-
-  FullDependence Result(Src, Dst, PossiblyLoopIndependent, CommonLevels);
-  ++TotalArrayPairs;
+  if (DstLoc.Size != SrcLoc.Size) {


Don't we need to check if they are precise or not (with calling isPrecise)?

kasuga-fj · 2025-04-11T15:15:33Z

llvm/lib/Analysis/DependenceAnalysis.cpp

+  if (DstLoc.Size != SrcLoc.Size) {
+    // The dependence test gets confused if the size of the memory accesses
+    // differ.
+    LLVM_DEBUG(dbgs() << "can't analyze must alias with different sizes\n");


Only "must alias"?

sebpop · 2025-05-09T12:57:23Z

This will be committed together with #123436

llvmbot added the llvm:analysis label Nov 18, 2024

sebpop mentioned this pull request Nov 18, 2024

Bug fixes for Array Dependence Analysis #116389

Closed

nikic reviewed Nov 19, 2024

View reviewed changes

sebpop force-pushed the da-2 branch from 4a15a58 to 150f723 Compare November 25, 2024 15:36

nikic reviewed Jan 7, 2025

View reviewed changes

sebpop force-pushed the da-2 branch from 150f723 to ee9862d Compare January 18, 2025 01:55

llvmbot added the testing-tools label Jan 18, 2025

sjoerdmeijer mentioned this pull request Jan 29, 2025

[LoopInterchange] Enable it by default (WIP) #124911

Open

[DA] Dependence analysis does not handle array accesses of different …

61f2003

…sizes This fixes bug llvm#16183 where the elements of a same array are accesses as i32 and i64. This is not handled by the array dependence analysis.

sebpop force-pushed the da-2 branch from ee9862d to 61f2003 Compare January 30, 2025 06:45

sebpop requested a review from Meinersbur February 25, 2025 16:38

Meinersbur mentioned this pull request Feb 26, 2025

[DA] handle memory accesses with different offsets and strides #123436

Merged

kasuga-fj reviewed Apr 11, 2025

View reviewed changes

sebpop closed this May 9, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DA] Dependence analysis does not handle array accesses of different sizes #116630

[DA] Dependence analysis does not handle array accesses of different sizes #116630

sebpop commented Nov 18, 2024

llvmbot commented Nov 18, 2024 •

edited

Loading

nikic left a comment

sebpop commented Jan 7, 2025

nikic left a comment

nikic Jan 7, 2025

nikic Jan 7, 2025

sebpop Jan 18, 2025

nikic Jan 7, 2025

github-actions bot commented Jan 18, 2025 •

edited

Loading

sebpop commented Jan 18, 2025

sebpop commented Jan 31, 2025

sebpop commented Feb 20, 2025

kasuga-fj Apr 11, 2025

kasuga-fj Apr 11, 2025

sebpop commented May 9, 2025

	define i64 @bug16183_alias(i32* nocapture %A) {
	define i64 @bug16183_alias(ptr nocapture %A) {

[DA] Dependence analysis does not handle array accesses of different sizes #116630

[DA] Dependence analysis does not handle array accesses of different sizes #116630

Conversation

sebpop commented Nov 18, 2024

llvmbot commented Nov 18, 2024 • edited Loading

nikic left a comment

Choose a reason for hiding this comment

sebpop commented Jan 7, 2025

nikic left a comment

Choose a reason for hiding this comment

nikic Jan 7, 2025

Choose a reason for hiding this comment

nikic Jan 7, 2025

Choose a reason for hiding this comment

sebpop Jan 18, 2025

Choose a reason for hiding this comment

nikic Jan 7, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 18, 2025 • edited Loading

sebpop commented Jan 18, 2025

sebpop commented Jan 31, 2025

sebpop commented Feb 20, 2025

kasuga-fj Apr 11, 2025

Choose a reason for hiding this comment

kasuga-fj Apr 11, 2025

Choose a reason for hiding this comment

sebpop commented May 9, 2025

llvmbot commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Jan 18, 2025 •

edited

Loading