[BasicAA] Fix handling of indirect assumption based results #100130

nikic · 2024-07-23T14:30:37Z

If a result is potentially based on a not yet proven assumption,
BasicAA will remember it inside AssumptionBasedResults and remove
the cache entry it an assumption higher up is later disproven.
However, we currently miss the case where another cache entry ends
up depending on such an AssumptionBased result.

Fix this by introducing an additional AssumptionBased state for
cache entries. If such a result is used, we'll still increment
AAQI.NumAssumptionUses, which means that the using entry will
also become AssumptionBased and be cleared if the assumption is
disproven.

At the end of the root query, convert remaining AssumptionBased
results into definitive results.

Fixes #98978.

llvmbot · 2024-07-23T14:31:12Z

@llvm/pr-subscribers-llvm-transforms

Author: Nikita Popov (nikic)

Changes

If a result is potentially based on a not yet proven assumption,
BasicAA will remember it inside AssumptionBasedResults and remove
the cache entry it an assumption higher up is later disproven.
However, we currently miss the case where another cache entry ends
up depending on such an AssumptionBased result.

Fix this by introducing an additional AssumptionBased state for
cache entries. If such a result is used, we'll still increment
AAQI.NumAssumptionUses, which means that the using entry will
also become AssumptionBased and be cleared if the assumption is
disproven.

At the end of the root query, convert remaining AssumptionBased
results into definitive results.

Fixes #98978.

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

3 Files Affected:

(modified) llvm/include/llvm/Analysis/AliasAnalysis.h (+14-3)
(modified) llvm/lib/Analysis/BasicAliasAnalysis.cpp (+24-4)
(added) llvm/test/Transforms/SLPVectorizer/X86/pr98978.ll (+127)

diff --git a/llvm/include/llvm/Analysis/AliasAnalysis.h b/llvm/include/llvm/Analysis/AliasAnalysis.h
index 812b5a9f72a3a..ceee03fcf97f6 100644
--- a/llvm/include/llvm/Analysis/AliasAnalysis.h
+++ b/llvm/include/llvm/Analysis/AliasAnalysis.h
@@ -244,12 +244,23 @@ class AAQueryInfo {
 public:
   using LocPair = std::pair<AACacheLoc, AACacheLoc>;
   struct CacheEntry {
+    /// Cache entry is neither an assumption nor does it use a (non-definitive)
+    /// assumption.
+    static constexpr int Definitive = -2;
+    /// Cache entry is not an assumption itself, may be using an assumption
+    /// from higher up the stack.
+    static constexpr int AssumptionBased = -1;
+
     AliasResult Result;
-    /// Number of times a NoAlias assumption has been used.
-    /// 0 for assumptions that have not been used, -1 for definitive results.
+    /// Number of times a NoAlias assumption has been used, 0 for assumptions
+    /// that have not been used. Can also take one of the Definitive or
+    /// AssumptionBased values documented above.
     int NumAssumptionUses;
+
     /// Whether this is a definitive (non-assumption) result.
-    bool isDefinitive() const { return NumAssumptionUses < 0; }
+    bool isDefinitive() const { return NumAssumptionUses == Definitive; }
+    /// Whether this is an assumption that has not been proven yet.
+    bool isAssumption() const { return NumAssumptionUses >= 0; }
   };
 
   // Alias analysis result aggregration using which this query is performed.
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index 161a3034e4829..e474899fb548e 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -1692,9 +1692,12 @@ AliasResult BasicAAResult::aliasCheck(const Value *V1, LocationSize V1Size,
   if (!Pair.second) {
     auto &Entry = Pair.first->second;
     if (!Entry.isDefinitive()) {
-      // Remember that we used an assumption.
-      ++Entry.NumAssumptionUses;
+      // Remember that we used an assumption. This may either be a direct use
+      // of an assumption, or a use of an entry that may itself be based on an
+      // assumption.
       ++AAQI.NumAssumptionUses;
+      if (Entry.isAssumption())
+        ++Entry.NumAssumptionUses;
     }
     // Cache contains sorted {V1,V2} pairs but we should return original order.
     auto Result = Entry.Result;
@@ -1722,7 +1725,6 @@ AliasResult BasicAAResult::aliasCheck(const Value *V1, LocationSize V1Size,
   Entry.Result = Result;
   // Cache contains sorted {V1,V2} pairs.
   Entry.Result.swap(Swapped);
-  Entry.NumAssumptionUses = -1;
 
   // If the assumption has been disproven, remove any results that may have
   // been based on this assumption. Do this after the Entry updates above to
@@ -1734,8 +1736,26 @@ AliasResult BasicAAResult::aliasCheck(const Value *V1, LocationSize V1Size,
   // The result may still be based on assumptions higher up in the chain.
   // Remember it, so it can be purged from the cache later.
   if (OrigNumAssumptionUses != AAQI.NumAssumptionUses &&
-      Result != AliasResult::MayAlias)
+      Result != AliasResult::MayAlias) {
     AAQI.AssumptionBasedResults.push_back(Locs);
+    Entry.NumAssumptionUses = AAQueryInfo::CacheEntry::AssumptionBased;
+  } else {
+    Entry.NumAssumptionUses = AAQueryInfo::CacheEntry::Definitive;
+  }
+
+  // Depth is incremented before this function is called, so Depth==1 indicates
+  // a root query.
+  if (AAQI.Depth == 1) {
+    // Any remaining assumption based results must be based on proven
+    // assumptions, so convert them to definitive results.
+    for (const auto &Loc : AAQI.AssumptionBasedResults) {
+      auto It = AAQI.AliasCache.find(Loc);
+      if (It != AAQI.AliasCache.end())
+        It->second.NumAssumptionUses = AAQueryInfo::CacheEntry::Definitive;
+    }
+    AAQI.AssumptionBasedResults.clear();
+    AAQI.NumAssumptionUses = 0;
+  }
   return Result;
 }
 
diff --git a/llvm/test/Transforms/SLPVectorizer/X86/pr98978.ll b/llvm/test/Transforms/SLPVectorizer/X86/pr98978.ll
new file mode 100644
index 0000000000000..a72c2b7d982d9
--- /dev/null
+++ b/llvm/test/Transforms/SLPVectorizer/X86/pr98978.ll
@@ -0,0 +1,127 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=slp-vectorizer < %s | FileCheck %s
+
+target triple = "x86_64-redhat-linux-gnu"
+
+; Should get vectorized.
+define void @test(ptr %arg, i64 %arg1, i64 %arg2) {
+; CHECK-LABEL: define void @test(
+; CHECK-SAME: ptr [[ARG:%.*]], i64 [[ARG1:%.*]], i64 [[ARG2:%.*]]) {
+; CHECK-NEXT:  [[_PREHEADER48_PREHEADER:.*]]:
+; CHECK-NEXT:    br label %[[DOTLOOPEXIT49:.*]]
+; CHECK:       [[DOTLOOPEXIT49]]:
+; CHECK-NEXT:    [[I:%.*]] = phi ptr [ [[I21:%.*]], %[[BB20:.*]] ], [ null, %[[_PREHEADER48_PREHEADER]] ]
+; CHECK-NEXT:    br i1 false, label %[[BB22:.*]], label %[[DOTPREHEADER48_PREHEADER:.*]]
+; CHECK:       [[_PREHEADER48_PREHEADER1:.*:]]
+; CHECK-NEXT:    br [[DOTLOOPEXIT50:label %.*]]
+; CHECK:       [[_LOOPEXIT49:.*]]:
+; CHECK-NEXT:    br [[DOTLOOPEXIT50]]
+; CHECK:       [[_PREHEADER48_PREHEADER_1:.*:]]
+; CHECK-NEXT:    [[I5:%.*]] = phi ptr [ [[I]], %[[_LOOPEXIT49]] ], [ [[I]], %[[DOTPREHEADER48_PREHEADER]] ]
+; CHECK-NEXT:    br label %[[DOTLOOPEXIT49_1:.*]]
+; CHECK:       [[_LOOPEXIT42_1:.*:]]
+; CHECK-NEXT:    br [[DOTLOOPEXIT49_2:label %.*]]
+; CHECK:       [[_LOOPEXIT49_1:.*:]]
+; CHECK-NEXT:    br i1 false, [[DOTLOOPEXIT49_2]], label %[[BB20]]
+; CHECK:       [[_LOOPEXIT49_2:.*:]]
+; CHECK-NEXT:    [[I6:%.*]] = phi ptr [ [[I5]], [[DOTLOOPEXIT42_1:%.*]] ], [ [[I5]], %[[DOTLOOPEXIT49_1]] ]
+; CHECK-NEXT:    [[I7:%.*]] = getelementptr inbounds i8, ptr [[I6]], i64 [[ARG1]]
+; CHECK-NEXT:    br label %[[BB8:.*]]
+; CHECK:       [[BB8]]:
+; CHECK-NEXT:    br label %[[BB10:.*]]
+; CHECK:       [[DEAD2:.*]]:
+; CHECK-NEXT:    br label %[[BB10]]
+; CHECK:       [[BB10]]:
+; CHECK-NEXT:    [[I11:%.*]] = phi ptr [ [[I7]], %[[BB8]] ], [ null, %[[DEAD2]] ]
+; CHECK-NEXT:    br label %[[BB13:.*]]
+; CHECK:       [[BB13]]:
+; CHECK-NEXT:    [[I14:%.*]] = phi ptr [ [[ARG]], %[[BB13]] ], [ [[I11]], %[[BB10]] ]
+; CHECK-NEXT:    [[I15:%.*]] = phi ptr [ null, %[[BB13]] ], [ [[I6]], %[[BB10]] ]
+; CHECK-NEXT:    [[I16:%.*]] = getelementptr inbounds i8, ptr [[I14]], i64 8
+; CHECK-NEXT:    [[I17:%.*]] = load i64, ptr [[I16]], align 1
+; CHECK-NEXT:    store i64 [[I17]], ptr [[I15]], align 1
+; CHECK-NEXT:    [[I18:%.*]] = getelementptr inbounds i8, ptr [[I15]], i64 8
+; CHECK-NEXT:    [[I19:%.*]] = load i64, ptr [[I14]], align 1
+; CHECK-NEXT:    store i64 [[I19]], ptr [[I18]], align 1
+; CHECK-NEXT:    br i1 false, label %[[BB13]], label %[[BB20]]
+; CHECK:       [[BB20]]:
+; CHECK-NEXT:    [[I21]] = phi ptr [ [[I5]], [[DOTLOOPEXIT42_1]] ], [ [[I6]], %[[BB13]] ]
+; CHECK-NEXT:    br label %[[DOTLOOPEXIT49]]
+; CHECK:       [[BB22]]:
+; CHECK-NEXT:    [[I23:%.*]] = getelementptr inbounds i8, ptr [[I]], i64 [[ARG2]]
+; CHECK-NEXT:    [[I25:%.*]] = getelementptr inbounds i8, ptr [[I23]], i64 8
+; CHECK-NEXT:    br label %[[BB26:.*]]
+; CHECK:       [[BB26]]:
+; CHECK-NEXT:    [[I27:%.*]] = phi ptr [ null, %[[BB26]] ], [ [[I25]], %[[BB22]] ]
+; CHECK-NEXT:    store i64 0, ptr [[I27]], align 1
+; CHECK-NEXT:    [[I28:%.*]] = getelementptr inbounds i8, ptr [[I27]], i64 8
+; CHECK-NEXT:    [[I29:%.*]] = load i64, ptr [[I23]], align 1
+; CHECK-NEXT:    store i64 0, ptr [[I28]], align 1
+; CHECK-NEXT:    br label %[[BB26]]
+;
+entry:
+  br label %loop1
+
+loop1:                                            ; preds = %bb20, %entry
+  %i = phi ptr [ %i21, %bb20 ], [ null, %entry ]
+  br i1 false, label %bb22, label %.preheader48.preheader
+
+.preheader48.preheader:                           ; preds = %loop1
+  br label %.loopexit49
+
+dead:                                             ; No predecessors!
+  br label %.loopexit49
+
+.loopexit49:                                      ; preds = %dead, %.preheader48.preheader
+  %i5 = phi ptr [ %i, %dead ], [ %i, %.preheader48.preheader ]
+  br label %.preheader48.preheader.1
+
+.preheader48.preheader.1:                         ; preds = %.loopexit49
+  br label %.loopexit49.1
+
+.loopexit42.1:                                    ; No predecessors!
+  br i1 false, label %.loopexit49.1, label %bb20
+
+.loopexit49.1:                                    ; preds = %.loopexit42.1, %.preheader48.preheader.1
+  %i6 = phi ptr [ %i5, %.loopexit42.1 ], [ %i5, %.preheader48.preheader.1 ]
+  %i7 = getelementptr inbounds i8, ptr %i6, i64 %arg1
+  br label %bb8
+
+bb8:                                              ; preds = %.loopexit49.1
+  br label %bb10
+
+dead2:                                            ; No predecessors!
+  br label %bb10
+
+bb10:                                             ; preds = %dead2, %bb8
+  %i11 = phi ptr [ %i7, %bb8 ], [ null, %dead2 ]
+  br label %bb13
+
+bb13:                                             ; preds = %bb13, %bb10
+  %i14 = phi ptr [ %arg, %bb13 ], [ %i11, %bb10 ]
+  %i15 = phi ptr [ null, %bb13 ], [ %i6, %bb10 ]
+  %i16 = getelementptr inbounds i8, ptr %i14, i64 8
+  %i17 = load i64, ptr %i16, align 1
+  store i64 %i17, ptr %i15, align 1
+  %i18 = getelementptr inbounds i8, ptr %i15, i64 8
+  %i19 = load i64, ptr %i14, align 1
+  store i64 %i19, ptr %i18, align 1
+  br i1 false, label %bb13, label %bb20
+
+bb20:                                             ; preds = %bb13, %.loopexit42.1
+  %i21 = phi ptr [ %i5, %.loopexit42.1 ], [ %i6, %bb13 ]
+  br label %loop1
+
+bb22:                                             ; preds = %loop1
+  %i23 = getelementptr inbounds i8, ptr %i, i64 %arg2
+  %i25 = getelementptr inbounds i8, ptr %i23, i64 8
+  br label %bb26
+
+bb26:                                             ; preds = %bb26, %bb22
+  %i27 = phi ptr [ null, %bb26 ], [ %i25, %bb22 ]
+  store i64 0, ptr %i27, align 1
+  %i28 = getelementptr inbounds i8, ptr %i27, i64 8
+  %i29 = load i64, ptr %i23, align 1
+  store i64 0, ptr %i28, align 1
+  br label %bb26
+}

llvmbot · 2024-07-23T14:31:13Z

@llvm/pr-subscribers-llvm-analysis

Author: Nikita Popov (nikic)

Changes

If a result is potentially based on a not yet proven assumption,
BasicAA will remember it inside AssumptionBasedResults and remove
the cache entry it an assumption higher up is later disproven.
However, we currently miss the case where another cache entry ends
up depending on such an AssumptionBased result.

Fix this by introducing an additional AssumptionBased state for
cache entries. If such a result is used, we'll still increment
AAQI.NumAssumptionUses, which means that the using entry will
also become AssumptionBased and be cleared if the assumption is
disproven.

At the end of the root query, convert remaining AssumptionBased
results into definitive results.

Fixes #98978.

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

3 Files Affected:

(modified) llvm/include/llvm/Analysis/AliasAnalysis.h (+14-3)
(modified) llvm/lib/Analysis/BasicAliasAnalysis.cpp (+24-4)
(added) llvm/test/Transforms/SLPVectorizer/X86/pr98978.ll (+127)

diff --git a/llvm/include/llvm/Analysis/AliasAnalysis.h b/llvm/include/llvm/Analysis/AliasAnalysis.h
index 812b5a9f72a3a..ceee03fcf97f6 100644
--- a/llvm/include/llvm/Analysis/AliasAnalysis.h
+++ b/llvm/include/llvm/Analysis/AliasAnalysis.h
@@ -244,12 +244,23 @@ class AAQueryInfo {
 public:
   using LocPair = std::pair<AACacheLoc, AACacheLoc>;
   struct CacheEntry {
+    /// Cache entry is neither an assumption nor does it use a (non-definitive)
+    /// assumption.
+    static constexpr int Definitive = -2;
+    /// Cache entry is not an assumption itself, may be using an assumption
+    /// from higher up the stack.
+    static constexpr int AssumptionBased = -1;
+
     AliasResult Result;
-    /// Number of times a NoAlias assumption has been used.
-    /// 0 for assumptions that have not been used, -1 for definitive results.
+    /// Number of times a NoAlias assumption has been used, 0 for assumptions
+    /// that have not been used. Can also take one of the Definitive or
+    /// AssumptionBased values documented above.
     int NumAssumptionUses;
+
     /// Whether this is a definitive (non-assumption) result.
-    bool isDefinitive() const { return NumAssumptionUses < 0; }
+    bool isDefinitive() const { return NumAssumptionUses == Definitive; }
+    /// Whether this is an assumption that has not been proven yet.
+    bool isAssumption() const { return NumAssumptionUses >= 0; }
   };
 
   // Alias analysis result aggregration using which this query is performed.
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index 161a3034e4829..e474899fb548e 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -1692,9 +1692,12 @@ AliasResult BasicAAResult::aliasCheck(const Value *V1, LocationSize V1Size,
   if (!Pair.second) {
     auto &Entry = Pair.first->second;
     if (!Entry.isDefinitive()) {
-      // Remember that we used an assumption.
-      ++Entry.NumAssumptionUses;
+      // Remember that we used an assumption. This may either be a direct use
+      // of an assumption, or a use of an entry that may itself be based on an
+      // assumption.
       ++AAQI.NumAssumptionUses;
+      if (Entry.isAssumption())
+        ++Entry.NumAssumptionUses;
     }
     // Cache contains sorted {V1,V2} pairs but we should return original order.
     auto Result = Entry.Result;
@@ -1722,7 +1725,6 @@ AliasResult BasicAAResult::aliasCheck(const Value *V1, LocationSize V1Size,
   Entry.Result = Result;
   // Cache contains sorted {V1,V2} pairs.
   Entry.Result.swap(Swapped);
-  Entry.NumAssumptionUses = -1;
 
   // If the assumption has been disproven, remove any results that may have
   // been based on this assumption. Do this after the Entry updates above to
@@ -1734,8 +1736,26 @@ AliasResult BasicAAResult::aliasCheck(const Value *V1, LocationSize V1Size,
   // The result may still be based on assumptions higher up in the chain.
   // Remember it, so it can be purged from the cache later.
   if (OrigNumAssumptionUses != AAQI.NumAssumptionUses &&
-      Result != AliasResult::MayAlias)
+      Result != AliasResult::MayAlias) {
     AAQI.AssumptionBasedResults.push_back(Locs);
+    Entry.NumAssumptionUses = AAQueryInfo::CacheEntry::AssumptionBased;
+  } else {
+    Entry.NumAssumptionUses = AAQueryInfo::CacheEntry::Definitive;
+  }
+
+  // Depth is incremented before this function is called, so Depth==1 indicates
+  // a root query.
+  if (AAQI.Depth == 1) {
+    // Any remaining assumption based results must be based on proven
+    // assumptions, so convert them to definitive results.
+    for (const auto &Loc : AAQI.AssumptionBasedResults) {
+      auto It = AAQI.AliasCache.find(Loc);
+      if (It != AAQI.AliasCache.end())
+        It->second.NumAssumptionUses = AAQueryInfo::CacheEntry::Definitive;
+    }
+    AAQI.AssumptionBasedResults.clear();
+    AAQI.NumAssumptionUses = 0;
+  }
   return Result;
 }
 
diff --git a/llvm/test/Transforms/SLPVectorizer/X86/pr98978.ll b/llvm/test/Transforms/SLPVectorizer/X86/pr98978.ll
new file mode 100644
index 0000000000000..a72c2b7d982d9
--- /dev/null
+++ b/llvm/test/Transforms/SLPVectorizer/X86/pr98978.ll
@@ -0,0 +1,127 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=slp-vectorizer < %s | FileCheck %s
+
+target triple = "x86_64-redhat-linux-gnu"
+
+; Should get vectorized.
+define void @test(ptr %arg, i64 %arg1, i64 %arg2) {
+; CHECK-LABEL: define void @test(
+; CHECK-SAME: ptr [[ARG:%.*]], i64 [[ARG1:%.*]], i64 [[ARG2:%.*]]) {
+; CHECK-NEXT:  [[_PREHEADER48_PREHEADER:.*]]:
+; CHECK-NEXT:    br label %[[DOTLOOPEXIT49:.*]]
+; CHECK:       [[DOTLOOPEXIT49]]:
+; CHECK-NEXT:    [[I:%.*]] = phi ptr [ [[I21:%.*]], %[[BB20:.*]] ], [ null, %[[_PREHEADER48_PREHEADER]] ]
+; CHECK-NEXT:    br i1 false, label %[[BB22:.*]], label %[[DOTPREHEADER48_PREHEADER:.*]]
+; CHECK:       [[_PREHEADER48_PREHEADER1:.*:]]
+; CHECK-NEXT:    br [[DOTLOOPEXIT50:label %.*]]
+; CHECK:       [[_LOOPEXIT49:.*]]:
+; CHECK-NEXT:    br [[DOTLOOPEXIT50]]
+; CHECK:       [[_PREHEADER48_PREHEADER_1:.*:]]
+; CHECK-NEXT:    [[I5:%.*]] = phi ptr [ [[I]], %[[_LOOPEXIT49]] ], [ [[I]], %[[DOTPREHEADER48_PREHEADER]] ]
+; CHECK-NEXT:    br label %[[DOTLOOPEXIT49_1:.*]]
+; CHECK:       [[_LOOPEXIT42_1:.*:]]
+; CHECK-NEXT:    br [[DOTLOOPEXIT49_2:label %.*]]
+; CHECK:       [[_LOOPEXIT49_1:.*:]]
+; CHECK-NEXT:    br i1 false, [[DOTLOOPEXIT49_2]], label %[[BB20]]
+; CHECK:       [[_LOOPEXIT49_2:.*:]]
+; CHECK-NEXT:    [[I6:%.*]] = phi ptr [ [[I5]], [[DOTLOOPEXIT42_1:%.*]] ], [ [[I5]], %[[DOTLOOPEXIT49_1]] ]
+; CHECK-NEXT:    [[I7:%.*]] = getelementptr inbounds i8, ptr [[I6]], i64 [[ARG1]]
+; CHECK-NEXT:    br label %[[BB8:.*]]
+; CHECK:       [[BB8]]:
+; CHECK-NEXT:    br label %[[BB10:.*]]
+; CHECK:       [[DEAD2:.*]]:
+; CHECK-NEXT:    br label %[[BB10]]
+; CHECK:       [[BB10]]:
+; CHECK-NEXT:    [[I11:%.*]] = phi ptr [ [[I7]], %[[BB8]] ], [ null, %[[DEAD2]] ]
+; CHECK-NEXT:    br label %[[BB13:.*]]
+; CHECK:       [[BB13]]:
+; CHECK-NEXT:    [[I14:%.*]] = phi ptr [ [[ARG]], %[[BB13]] ], [ [[I11]], %[[BB10]] ]
+; CHECK-NEXT:    [[I15:%.*]] = phi ptr [ null, %[[BB13]] ], [ [[I6]], %[[BB10]] ]
+; CHECK-NEXT:    [[I16:%.*]] = getelementptr inbounds i8, ptr [[I14]], i64 8
+; CHECK-NEXT:    [[I17:%.*]] = load i64, ptr [[I16]], align 1
+; CHECK-NEXT:    store i64 [[I17]], ptr [[I15]], align 1
+; CHECK-NEXT:    [[I18:%.*]] = getelementptr inbounds i8, ptr [[I15]], i64 8
+; CHECK-NEXT:    [[I19:%.*]] = load i64, ptr [[I14]], align 1
+; CHECK-NEXT:    store i64 [[I19]], ptr [[I18]], align 1
+; CHECK-NEXT:    br i1 false, label %[[BB13]], label %[[BB20]]
+; CHECK:       [[BB20]]:
+; CHECK-NEXT:    [[I21]] = phi ptr [ [[I5]], [[DOTLOOPEXIT42_1]] ], [ [[I6]], %[[BB13]] ]
+; CHECK-NEXT:    br label %[[DOTLOOPEXIT49]]
+; CHECK:       [[BB22]]:
+; CHECK-NEXT:    [[I23:%.*]] = getelementptr inbounds i8, ptr [[I]], i64 [[ARG2]]
+; CHECK-NEXT:    [[I25:%.*]] = getelementptr inbounds i8, ptr [[I23]], i64 8
+; CHECK-NEXT:    br label %[[BB26:.*]]
+; CHECK:       [[BB26]]:
+; CHECK-NEXT:    [[I27:%.*]] = phi ptr [ null, %[[BB26]] ], [ [[I25]], %[[BB22]] ]
+; CHECK-NEXT:    store i64 0, ptr [[I27]], align 1
+; CHECK-NEXT:    [[I28:%.*]] = getelementptr inbounds i8, ptr [[I27]], i64 8
+; CHECK-NEXT:    [[I29:%.*]] = load i64, ptr [[I23]], align 1
+; CHECK-NEXT:    store i64 0, ptr [[I28]], align 1
+; CHECK-NEXT:    br label %[[BB26]]
+;
+entry:
+  br label %loop1
+
+loop1:                                            ; preds = %bb20, %entry
+  %i = phi ptr [ %i21, %bb20 ], [ null, %entry ]
+  br i1 false, label %bb22, label %.preheader48.preheader
+
+.preheader48.preheader:                           ; preds = %loop1
+  br label %.loopexit49
+
+dead:                                             ; No predecessors!
+  br label %.loopexit49
+
+.loopexit49:                                      ; preds = %dead, %.preheader48.preheader
+  %i5 = phi ptr [ %i, %dead ], [ %i, %.preheader48.preheader ]
+  br label %.preheader48.preheader.1
+
+.preheader48.preheader.1:                         ; preds = %.loopexit49
+  br label %.loopexit49.1
+
+.loopexit42.1:                                    ; No predecessors!
+  br i1 false, label %.loopexit49.1, label %bb20
+
+.loopexit49.1:                                    ; preds = %.loopexit42.1, %.preheader48.preheader.1
+  %i6 = phi ptr [ %i5, %.loopexit42.1 ], [ %i5, %.preheader48.preheader.1 ]
+  %i7 = getelementptr inbounds i8, ptr %i6, i64 %arg1
+  br label %bb8
+
+bb8:                                              ; preds = %.loopexit49.1
+  br label %bb10
+
+dead2:                                            ; No predecessors!
+  br label %bb10
+
+bb10:                                             ; preds = %dead2, %bb8
+  %i11 = phi ptr [ %i7, %bb8 ], [ null, %dead2 ]
+  br label %bb13
+
+bb13:                                             ; preds = %bb13, %bb10
+  %i14 = phi ptr [ %arg, %bb13 ], [ %i11, %bb10 ]
+  %i15 = phi ptr [ null, %bb13 ], [ %i6, %bb10 ]
+  %i16 = getelementptr inbounds i8, ptr %i14, i64 8
+  %i17 = load i64, ptr %i16, align 1
+  store i64 %i17, ptr %i15, align 1
+  %i18 = getelementptr inbounds i8, ptr %i15, i64 8
+  %i19 = load i64, ptr %i14, align 1
+  store i64 %i19, ptr %i18, align 1
+  br i1 false, label %bb13, label %bb20
+
+bb20:                                             ; preds = %bb13, %.loopexit42.1
+  %i21 = phi ptr [ %i5, %.loopexit42.1 ], [ %i6, %bb13 ]
+  br label %loop1
+
+bb22:                                             ; preds = %loop1
+  %i23 = getelementptr inbounds i8, ptr %i, i64 %arg2
+  %i25 = getelementptr inbounds i8, ptr %i23, i64 8
+  br label %bb26
+
+bb26:                                             ; preds = %bb26, %bb22
+  %i27 = phi ptr [ null, %bb26 ], [ %i25, %bb22 ]
+  store i64 0, ptr %i27, align 1
+  %i28 = getelementptr inbounds i8, ptr %i27, i64 8
+  %i29 = load i64, ptr %i23, align 1
+  store i64 0, ptr %i28, align 1
+  br label %bb26
+}

nikic · 2024-07-23T14:32:49Z

llvm/test/Transforms/SLPVectorizer/X86/pr98978.ll

This test is a slightly reduced version from the original report in #98978. Unfortunately, the fact that this issue requires use of BatchAA makes it pretty hard to test, as one needs a specific interaction of IR and query order.

The test and the changes looks good

PR Link: llvm/llvm-project#100130

fhahn · 2024-07-24T07:50:40Z

llvm/test/Transforms/SLPVectorizer/X86/pr98978.ll

+  br label %loop1
+
+loop1:                                            ; preds = %bb20, %entry
+  %i = phi ptr [ %i21, %bb20 ], [ null, %entry ]


Might be good to avoid null pointers here and elsewhere if possible?

I was able to get rid of one of the null pointers, but without the others the issue doesn't reproduce.

fhahn · 2024-07-24T07:51:01Z

llvm/test/Transforms/SLPVectorizer/X86/pr98978.ll

@@ -0,0 +1,127 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=slp-vectorizer < %s | FileCheck %s


Should we also have a BasicAA analysis test based on this?

The BasicAA analysis tests don't use BatchAA. I think it might be possible to construct a case where this issue also occurs without BatchAA, but it's not easy.

Ah that's a shame, relying on SLP seems a bit fragile, but it sounds like it's the best option we have currently.

If a result is potentially based on a not yet proven assumption, BasicAA will remember it inside AssumptionBasedResults and remove the cache entry it an assumption higher up is later disproven. However, we currently miss the case where another cache entry ends up depending on such an AssumptionBased result. Fix this by introducing an additional AssumptionBased state for cache entries. If such a result is used, we'll still increment AAQI.NumAssumptionUses, which means that the using entry will also become AssumptionBased and be cleared if the assumption is disproven. At the end of the root query, convert remaining AssumptionBased results into definitive results. Fixes llvm#98978.

fhahn · 2024-07-24T15:46:10Z

llvm/test/Transforms/SLPVectorizer/X86/pr98978.ll

@@ -0,0 +1,127 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=slp-vectorizer < %s | FileCheck %s


Ah that's a shame, relying on SLP seems a bit fragile, but it sounds like it's the best option we have currently.

fhahn · 2024-07-24T15:46:39Z

llvm/test/Transforms/SLPVectorizer/X86/pr98978.ll

+
+target triple = "x86_64-redhat-linux-gnu"
+
+; Should not get vectorized.


Would probably be helpful to include a bit more info about why it shouldn't get vectorized, as the test case isn't super easy to read.

fhahn

LGTM, thanks!

) If a result is potentially based on a not yet proven assumption, BasicAA will remember it inside AssumptionBasedResults and remove the cache entry if an assumption higher up is later disproved. However, we currently miss the case where another cache entry ends up depending on such an AssumptionBased result. Fix this by introducing an additional AssumptionBased state for cache entries. If such a result is used, we'll still increment AAQI.NumAssumptionUses, which means that the using entry will also become AssumptionBased and be cleared if the assumption is disproved. At the end of the root query, convert remaining AssumptionBased results into definitive results. Fixes llvm#98978. (cherry picked from commit 9107338)

Summary: If a result is potentially based on a not yet proven assumption, BasicAA will remember it inside AssumptionBasedResults and remove the cache entry if an assumption higher up is later disproved. However, we currently miss the case where another cache entry ends up depending on such an AssumptionBased result. Fix this by introducing an additional AssumptionBased state for cache entries. If such a result is used, we'll still increment AAQI.NumAssumptionUses, which means that the using entry will also become AssumptionBased and be cleared if the assumption is disproved. At the end of the root query, convert remaining AssumptionBased results into definitive results. Fixes #98978. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250771

) If a result is potentially based on a not yet proven assumption, BasicAA will remember it inside AssumptionBasedResults and remove the cache entry if an assumption higher up is later disproved. However, we currently miss the case where another cache entry ends up depending on such an AssumptionBased result. Fix this by introducing an additional AssumptionBased state for cache entries. If such a result is used, we'll still increment AAQI.NumAssumptionUses, which means that the using entry will also become AssumptionBased and be cleared if the assumption is disproved. At the end of the root query, convert remaining AssumptionBased results into definitive results. Fixes llvm#98978. (cherry picked from commit 9107338)

nikic requested review from fhahn, alinas and dtcxzyw July 23, 2024 14:30

llvmbot added llvm:analysis llvm:transforms labels Jul 23, 2024

nikic force-pushed the basicaa-assumption-based branch from b32858d to c256c34 Compare July 23, 2024 14:31

nikic commented Jul 23, 2024

View reviewed changes

nikic force-pushed the basicaa-assumption-based branch from c256c34 to 492c929 Compare July 23, 2024 14:35

nikic mentioned this pull request Jul 23, 2024

[BasicAA] Incorrect alias analysis causing miscompile in slp-vectorizer #98978

Closed

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Jul 23, 2024

pre-commit: test PR100130

50ffd7b

PR Link: llvm/llvm-project#100130

dtcxzyw mentioned this pull request Jul 23, 2024

pre-commit: test PR100130 dtcxzyw/llvm-opt-benchmark#1046

Closed

fhahn reviewed Jul 24, 2024

View reviewed changes

nikic added 2 commits July 24, 2024 14:56

[SLP] Precommit test for llvm#98978 (NFC)

a8fbbbd

nikic force-pushed the basicaa-assumption-based branch from 492c929 to 386918b Compare July 24, 2024 12:58

fhahn reviewed Jul 24, 2024

View reviewed changes

Expand comment

8fac86f

fhahn approved these changes Jul 25, 2024

View reviewed changes

nikic merged commit 9107338 into llvm:main Jul 25, 2024
7 checks passed

nikic deleted the basicaa-assumption-based branch July 25, 2024 10:25

philnik777 mentioned this pull request Aug 2, 2024

[Clang] Add a release note deprecating __is_nullptr #101638

Merged

mgabka mentioned this pull request Aug 22, 2024

Add release note about ABI mgabka/llvm-project#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BasicAA] Fix handling of indirect assumption based results #100130

[BasicAA] Fix handling of indirect assumption based results #100130

nikic commented Jul 23, 2024

llvmbot commented Jul 23, 2024

llvmbot commented Jul 23, 2024

nikic Jul 23, 2024

alexey-bataev Jul 23, 2024

fhahn Jul 24, 2024

nikic Jul 24, 2024

fhahn Jul 24, 2024

nikic Jul 24, 2024

fhahn Jul 24, 2024

fhahn Jul 24, 2024

fhahn Jul 24, 2024

fhahn left a comment

		@@ -0,0 +1,127 @@
		; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
		; RUN: opt -S -passes=slp-vectorizer < %s \| FileCheck %s


		target triple = "x86_64-redhat-linux-gnu"

		; Should not get vectorized.

[BasicAA] Fix handling of indirect assumption based results #100130

[BasicAA] Fix handling of indirect assumption based results #100130

Conversation

nikic commented Jul 23, 2024

llvmbot commented Jul 23, 2024

llvmbot commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhahn left a comment

Choose a reason for hiding this comment