Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Jul 25, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a8fbbbd
[SLP] Precommit test for #98978 (NFC)
nikic Jul 23, 2024
386918b
[BasicAA] Fix handling of indirect assumption based results
nikic Jul 24, 2024
8fac86f
Expand comment
nikic Jul 25, 2024
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
17 changes: 14 additions & 3 deletions llvm/include/llvm/Analysis/AliasAnalysis.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, but 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.
Expand Down
28 changes: 24 additions & 4 deletions llvm/lib/Analysis/BasicAliasAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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;
}

Expand Down
103 changes: 103 additions & 0 deletions llvm/test/Transforms/SLPVectorizer/X86/pr98978.ll
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

The test and the changes looks good

Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
; RUN: opt -S -passes=slp-vectorizer < %s | FileCheck %s
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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


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

; Should not get vectorized.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

define void @test(ptr %p1, i64 %arg1, i64 %arg2) {
; CHECK-LABEL: define void @test(
; CHECK-SAME: ptr [[P1:%.*]], i64 [[ARG1:%.*]], i64 [[ARG2:%.*]]) {
; CHECK-NEXT: [[_PREHEADER48_PREHEADER_1:.*]]:
; CHECK-NEXT: br label %[[_LOOPEXIT49_1:.*]]
; CHECK: [[_LOOPEXIT49_1]]:
; CHECK-NEXT: [[I:%.*]] = phi ptr [ [[I21:%.*]], %[[BB20:.*]] ], [ [[P1]], %[[_PREHEADER48_PREHEADER_1]] ]
; CHECK-NEXT: br i1 false, label %[[BB22:.*]], label %[[DOTPREHEADER48_PREHEADER_1:.*]]
; CHECK: [[DEAD:.*]]:
; CHECK-NEXT: br label %[[DOTPREHEADER48_PREHEADER_1]]
; CHECK: [[_PREHEADER48_PREHEADER_2:.*:]]
; CHECK-NEXT: [[I5:%.*]] = phi ptr [ [[I]], %[[DEAD]] ], [ [[I]], %[[_LOOPEXIT49_1]] ]
; CHECK-NEXT: br label %[[DOTLOOPEXIT49_1:.*]]
; CHECK: [[DEAD1:.*]]:
; CHECK-NEXT: br i1 false, label %[[DOTLOOPEXIT49_1]], label %[[BB20]]
; CHECK: [[_LOOPEXIT49_2:.*:]]
; CHECK-NEXT: [[I6:%.*]] = phi ptr [ [[I5]], %[[DEAD1]] ], [ [[I5]], %[[DOTPREHEADER48_PREHEADER_1]] ]
; CHECK-NEXT: [[I7:%.*]] = getelementptr i8, ptr [[I6]], i64 [[ARG1]]
; CHECK-NEXT: br label %[[BB10:.*]]
; CHECK: [[DEAD2:.*]]:
; CHECK-NEXT: br label %[[BB10]]
; CHECK: [[BB10]]:
; CHECK-NEXT: [[I11:%.*]] = phi ptr [ [[I7]], %[[DOTLOOPEXIT49_1]] ], [ null, %[[DEAD2]] ]
; CHECK-NEXT: [[I16:%.*]] = getelementptr i8, ptr [[I11]], i64 8
; CHECK-NEXT: [[I17:%.*]] = load i64, ptr [[I16]], align 1
; CHECK-NEXT: store i64 [[I17]], ptr [[I6]], align 1
; CHECK-NEXT: [[I18:%.*]] = getelementptr i8, ptr [[I6]], i64 8
; CHECK-NEXT: [[I19:%.*]] = load i64, ptr [[I11]], align 1
; CHECK-NEXT: store i64 [[I19]], ptr [[I18]], align 1
; CHECK-NEXT: br label %[[BB20]]
; CHECK: [[BB20]]:
; CHECK-NEXT: [[I21]] = phi ptr [ [[I5]], %[[DEAD1]] ], [ [[I6]], %[[BB10]] ]
; CHECK-NEXT: br label %[[_LOOPEXIT49_1]]
; CHECK: [[BB22]]:
; CHECK-NEXT: [[I23:%.*]] = getelementptr i8, ptr [[I]], i64 [[ARG2]]
; CHECK-NEXT: [[I25:%.*]] = getelementptr 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 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 ], [ %p1, %entry ]
br i1 false, label %bb22, label %.preheader48.preheader.1

dead: ; No predecessors!
br label %.preheader48.preheader.1

.preheader48.preheader.1: ; preds = %dead, %loop1
%i5 = phi ptr [ %i, %dead ], [ %i, %loop1 ]
br label %.loopexit49.1

dead1: ; No predecessors!
br i1 false, label %.loopexit49.1, label %bb20

.loopexit49.1: ; preds = %dead1, %.preheader48.preheader.1
%i6 = phi ptr [ %i5, %dead1 ], [ %i5, %.preheader48.preheader.1 ]
%i7 = getelementptr i8, ptr %i6, i64 %arg1
br label %bb10

dead2: ; No predecessors!
br label %bb10

bb10: ; preds = %dead2, %.loopexit49.1
%i11 = phi ptr [ %i7, %.loopexit49.1 ], [ null, %dead2 ]
%i16 = getelementptr i8, ptr %i11, i64 8
%i17 = load i64, ptr %i16, align 1
store i64 %i17, ptr %i6, align 1
%i18 = getelementptr i8, ptr %i6, i64 8
%i19 = load i64, ptr %i11, align 1
store i64 %i19, ptr %i18, align 1
br label %bb20

bb20: ; preds = %bb10, %dead1
%i21 = phi ptr [ %i5, %dead1 ], [ %i6, %bb10 ]
br label %loop1

bb22: ; preds = %loop1
%i23 = getelementptr i8, ptr %i, i64 %arg2
%i25 = getelementptr 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 i8, ptr %i27, i64 8
%i29 = load i64, ptr %i23, align 1
store i64 0, ptr %i28, align 1
br label %bb26
}
Loading