[llvm] [AST] Don't merge memory locations in AliasSetTracker (PR #65731)
via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 12 03:47:55 PDT 2023
llvmbot wrote:
@llvm/pr-subscribers-llvm-transforms
<details>
<summary>Changes</summary>
This changes the AliasSetTracker to track memory locations instead of pointers in its alias sets. The motivation for this is outlined in [an RFC posted on LLVM discourse](https://discourse.llvm.org/t/rfc-dont-merge-memory-locations-in-aliassettracker/73336).
This commit does not yet change method names and code comments to refer to "memory location(s)" instead of "pointer(s)", to keep the changeset focused, and to first gain feedback on the idea. An additional commit can take care of this.
In the data structures of the AST implementation, I made the choice to replace the linked list of `PointerRec` entries (that had to go anyway) with a simple flat vector of `MemoryLocation` objects, but for the `AliasSet` objects referenced from a lookup table, I retained the mechanism of a linked list, reference counting, forwarding, etc. The data structures could be revised in a follow-up change.
--
Patch is 114.78 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/65731.diff
16 Files Affected:
- (modified) llvm/include/llvm/Analysis/AliasSetTracker.h (+17-161)
- (modified) llvm/include/llvm/LinkAllPasses.h (+1-2)
- (modified) llvm/lib/Analysis/AliasSetTracker.cpp (+81-159)
- (modified) llvm/lib/Analysis/LoopAccessAnalysis.cpp (+5-5)
- (modified) llvm/lib/Transforms/Scalar/LICM.cpp (+1-1)
- (modified) llvm/lib/Transforms/Scalar/LoopVersioningLICM.cpp (+3-3)
- (modified) llvm/test/Analysis/AliasSet/argmemonly.ll (+16-16)
- (modified) llvm/test/Analysis/AliasSet/guards.ll (+108-108)
- (modified) llvm/test/Analysis/AliasSet/intrinsics.ll (+5-5)
- (modified) llvm/test/Analysis/AliasSet/memset.ll (+4-4)
- (modified) llvm/test/Analysis/AliasSet/memtransfer.ll (+14-14)
- (modified) llvm/test/Analysis/AliasSet/saturation.ll (+15-17)
- (added) llvm/test/Transforms/LICM/variant-aainfo.ll (+65)
- (modified) polly/lib/Analysis/ScopBuilder.cpp (+4-2)
- (modified) polly/lib/Analysis/ScopDetection.cpp (+2-2)
- (modified) polly/lib/Analysis/ScopDetectionDiagnostic.cpp (+2-1)
<pre>
diff --git a/llvm/include/llvm/Analysis/AliasSetTracker.h b/llvm/include/llvm/Analysis/AliasSetTracker.h
index e485e1ff2f4c987..fe4e6e7e7acba91 100644
--- a/llvm/include/llvm/Analysis/AliasSetTracker.h
+++ b/llvm/include/llvm/Analysis/AliasSetTracker.h
@@ -49,99 +49,12 @@ class Value;
class AliasSet : public ilist_node<AliasSet> {
friend class AliasSetTracker;
- class PointerRec {
- Value *Val; // The pointer this record corresponds to.
- PointerRec **PrevInList = nullptr;
- PointerRec *NextInList = nullptr;
- AliasSet *AS = nullptr;
- LocationSize Size = LocationSize::mapEmpty();
- AAMDNodes AAInfo;
-
- // Whether the size for this record has been set at all. This makes no
- // guarantees about the size being known.
- bool isSizeSet() const { return Size != LocationSize::mapEmpty(); }
-
- public:
- PointerRec(Value *V)
- : Val(V), AAInfo(DenseMapInfo<AAMDNodes>::getEmptyKey()) {}
-
- Value *getValue() const { return Val; }
-
- PointerRec *getNext() const { return NextInList; }
- bool hasAliasSet() const { return AS != nullptr; }
-
- PointerRec** setPrevInList(PointerRec **PIL) {
- PrevInList = PIL;
- return &NextInList;
- }
-
- bool updateSizeAndAAInfo(LocationSize NewSize, const AAMDNodes &NewAAInfo) {
- bool SizeChanged = false;
- if (NewSize != Size) {
- LocationSize OldSize = Size;
- Size = isSizeSet() ? Size.unionWith(NewSize) : NewSize;
- SizeChanged = OldSize != Size;
- }
-
- if (AAInfo == DenseMapInfo<AAMDNodes>::getEmptyKey())
- // We don't have a AAInfo yet. Set it to NewAAInfo.
- AAInfo = NewAAInfo;
- else {
- AAMDNodes Intersection(AAInfo.intersect(NewAAInfo));
- SizeChanged |= Intersection != AAInfo;
- AAInfo = Intersection;
- }
- return SizeChanged;
- }
-
- LocationSize getSize() const {
- assert(isSizeSet() && "Getting an unset size!");
- return Size;
- }
-
- /// Return the AAInfo, or null if there is no information or conflicting
- /// information.
- AAMDNodes getAAInfo() const {
- // If we have missing or conflicting AAInfo, return null.
- if (AAInfo == DenseMapInfo<AAMDNodes>::getEmptyKey() ||
- AAInfo == DenseMapInfo<AAMDNodes>::getTombstoneKey())
- return AAMDNodes();
- return AAInfo;
- }
-
- AliasSet *getAliasSet(AliasSetTracker &AST) {
- assert(AS && "No AliasSet yet!");
- if (AS->Forward) {
- AliasSet *OldAS = AS;
- AS = OldAS->getForwardedTarget(AST);
- AS->addRef();
- OldAS->dropRef(AST);
- }
- return AS;
- }
-
- void setAliasSet(AliasSet *as) {
- assert(!AS && "Already have an alias set!");
- AS = as;
- }
-
- void eraseFromList() {
- if (NextInList) NextInList->PrevInList = PrevInList;
- *PrevInList = NextInList;
- if (AS->PtrListEnd == &NextInList) {
- AS->PtrListEnd = PrevInList;
- assert(*AS->PtrListEnd == nullptr && "List not terminated right!");
- }
- delete this;
- }
- };
-
- // Doubly linked list of nodes.
- PointerRec *PtrList = nullptr;
- PointerRec **PtrListEnd;
// Forwarding pointer.
AliasSet *Forward = nullptr;
+ /// Memory locations in this alias set.
+ std::vector<MemoryLocation> MemoryLocs;
+
/// All instructions without a specific address in this alias set.
std::vector<AssertingVH<Instruction>> UnknownInsts;
@@ -178,8 +91,6 @@ class AliasSet : public ilist_node<AliasSet> {
};
unsigned Alias : 1;
- unsigned SetSize = 0;
-
void addRef() { ++RefCount; }
void dropRef(AliasSetTracker &AST) {
@@ -207,66 +118,21 @@ class AliasSet : public ilist_node<AliasSet> {
// Alias Set iteration - Allow access to all of the pointers which are part of
// this alias set.
- class iterator;
- iterator begin() const { return iterator(PtrList); }
- iterator end() const { return iterator(); }
- bool empty() const { return PtrList == nullptr; }
+ using iterator = std::vector<MemoryLocation>::const_iterator;
+ iterator begin() const { return MemoryLocs.begin(); }
+ iterator end() const { return MemoryLocs.end(); }
- // Unfortunately, ilist::size() is linear, so we have to add code to keep
- // track of the list's exact size.
- unsigned size() { return SetSize; }
+ unsigned size() { return MemoryLocs.size(); }
void print(raw_ostream &OS) const;
void dump() const;
- /// Define an iterator for alias sets... this is just a forward iterator.
- class iterator {
- PointerRec *CurNode;
-
- public:
- using iterator_category = std::forward_iterator_tag;
- using value_type = PointerRec;
- using difference_type = std::ptrdiff_t;
- using pointer = value_type *;
- using reference = value_type &;
-
- explicit iterator(PointerRec *CN = nullptr) : CurNode(CN) {}
-
- bool operator==(const iterator& x) const {
- return CurNode == x.CurNode;
- }
- bool operator!=(const iterator& x) const { return !operator==(x); }
-
- value_type &operator*() const {
- assert(CurNode && "Dereferencing AliasSet.end()!");
- return *CurNode;
- }
- value_type *operator->() const { return &operator*(); }
-
- Value *getPointer() const { return CurNode->getValue(); }
- LocationSize getSize() const { return CurNode->getSize(); }
- AAMDNodes getAAInfo() const { return CurNode->getAAInfo(); }
-
- iterator& operator++() { // Preincrement
- assert(CurNode && "Advancing past AliasSet.end()!");
- CurNode = CurNode->getNext();
- return *this;
- }
- iterator operator++(int) { // Postincrement
- iterator tmp = *this; ++*this; return tmp;
- }
- };
-
private:
// Can only be created by AliasSetTracker.
AliasSet()
- : PtrListEnd(&PtrList), RefCount(0), AliasAny(false), Access(NoAccess),
+ : RefCount(0), AliasAny(false), Access(NoAccess),
Alias(SetMustAlias) {}
- PointerRec *getSomePointer() const {
- return PtrList;
- }
-
/// Return the real alias set this represents. If this has been merged with
/// another set and is forwarding, return the ultimate destination set. This
/// also implements the union-find collapsing as well.
@@ -284,16 +150,16 @@ class AliasSet : public ilist_node<AliasSet> {
void removeFromTracker(AliasSetTracker &AST);
- void addPointer(AliasSetTracker &AST, PointerRec &Entry, LocationSize Size,
- const AAMDNodes &AAInfo, bool KnownMustAlias = false,
- bool SkipSizeUpdate = false);
+ bool isMustAliasMergeWith(AliasSet &AS, BatchAAResults &BatchAA) const;
+ void addPointer(AliasSetTracker &AST, const MemoryLocation &MemLoc,
+ bool KnownMustAlias = false);
void addUnknownInst(Instruction *I, BatchAAResults &AA);
public:
/// If the specified pointer "may" (or must) alias one of the members in the
/// set return the appropriate AliasResult. Otherwise return NoAlias.
- AliasResult aliasesPointer(const Value *Ptr, LocationSize Size,
- const AAMDNodes &AAInfo, BatchAAResults &AA) const;
+ AliasResult aliasesPointer(const MemoryLocation &MemLoc, BatchAAResults &AA) const;
+
ModRefInfo aliasesUnknownInst(const Instruction *Inst,
BatchAAResults &AA) const;
};
@@ -307,7 +173,7 @@ class AliasSetTracker {
BatchAAResults &AA;
ilist<AliasSet> AliasSets;
- using PointerMapType = DenseMap<AssertingVH<Value>, AliasSet::PointerRec *>;
+ using PointerMapType = DenseMap<MemoryLocation, AliasSet *>;
// Map from pointers to their node
PointerMapType PointerMap;
@@ -330,7 +196,7 @@ class AliasSetTracker {
/// These methods return true if inserting the instruction resulted in the
/// addition of a new alias set (i.e., the pointer did not alias anything).
///
- void add(Value *Ptr, LocationSize Size, const AAMDNodes &AAInfo); // Add a loc
+ void add(const MemoryLocation &Loc); // Add a loc
void add(LoadInst *LI);
void add(StoreInst *SI);
void add(VAArgInst *VAAI);
@@ -371,7 +237,7 @@ class AliasSetTracker {
friend class AliasSet;
// The total number of pointers contained in all "may" alias sets.
- unsigned TotalMayAliasSetSize = 0;
+ unsigned TotalAliasSetSize = 0;
// A non-null value signifies this AST is saturated. A saturated AST lumps
// all pointers into a single "May" set.
@@ -379,18 +245,8 @@ class AliasSetTracker {
void removeAliasSet(AliasSet *AS);
- /// Just like operator[] on the map, except that it creates an entry for the
- /// pointer if it doesn't already exist.
- AliasSet::PointerRec &getEntryFor(Value *V) {
- AliasSet::PointerRec *&Entry = PointerMap[V];
- if (!Entry)
- Entry = new AliasSet::PointerRec(V);
- return *Entry;
- }
-
AliasSet &addPointer(MemoryLocation Loc, AliasSet::AccessLattice E);
- AliasSet *mergeAliasSetsForPointer(const Value *Ptr, LocationSize Size,
- const AAMDNodes &AAInfo,
+ AliasSet *mergeAliasSetsForPointer(const MemoryLocation& MemLoc,
bool &MustAliasAll);
/// Merge all alias sets into a single set that is considered to alias any
diff --git a/llvm/include/llvm/LinkAllPasses.h b/llvm/include/llvm/LinkAllPasses.h
index 081a15077e7be4c..a4ec0b337129787 100644
--- a/llvm/include/llvm/LinkAllPasses.h
+++ b/llvm/include/llvm/LinkAllPasses.h
@@ -163,8 +163,7 @@ namespace {
llvm::AliasAnalysis AA(TLI);
llvm::BatchAAResults BAA(AA);
llvm::AliasSetTracker X(BAA);
- X.add(nullptr, llvm::LocationSize::beforeOrAfterPointer(),
- llvm::AAMDNodes()); // for -print-alias-sets
+ X.add(llvm::MemoryLocation()); // for -print-alias-sets
(void) llvm::AreStatisticsEnabled();
(void) llvm::sys::RunningOnValgrind();
}
diff --git a/llvm/lib/Analysis/AliasSetTracker.cpp b/llvm/lib/Analysis/AliasSetTracker.cpp
index 91b889116dfa2d3..f5554b193694539 100644
--- a/llvm/lib/Analysis/AliasSetTracker.cpp
+++ b/llvm/lib/Analysis/AliasSetTracker.cpp
@@ -40,36 +40,37 @@ static cl::opt<unsigned>
cl::desc("The maximum number of pointers may-alias "
"sets may contain before degradation"));
+bool AliasSet::isMustAliasMergeWith(AliasSet &AS, BatchAAResults &BatchAA) const {
+ for (const MemoryLocation &MemLoc : MemoryLocs)
+ for (const MemoryLocation &ASMemLoc : AS.MemoryLocs)
+ if (!BatchAA.isMustAlias(MemLoc, ASMemLoc))
+ return false;
+ return true;
+}
+
/// mergeSetIn - Merge the specified alias set into this alias set.
void AliasSet::mergeSetIn(AliasSet &AS, AliasSetTracker &AST,
BatchAAResults &BatchAA) {
assert(!AS.Forward && "Alias set is already forwarding!");
assert(!Forward && "This set is a forwarding set!!");
- bool WasMustAlias = (Alias == SetMustAlias);
// Update the alias and access types of this set...
Access |= AS.Access;
Alias |= AS.Alias;
if (Alias == SetMustAlias) {
- // Check that these two merged sets really are must aliases. Since both
- // used to be must-alias sets, we can just check any pointer from each set
- // for aliasing.
- PointerRec *L = getSomePointer();
- PointerRec *R = AS.getSomePointer();
-
+ // Check that these two merged sets really are must aliases.
// If the pointers are not a must-alias pair, this set becomes a may alias.
- if (!BatchAA.isMustAlias(
- MemoryLocation(L->getValue(), L->getSize(), L->getAAInfo()),
- MemoryLocation(R->getValue(), R->getSize(), R->getAAInfo())))
+ if (!isMustAliasMergeWith(AS, BatchAA))
Alias = SetMayAlias;
}
- if (Alias == SetMayAlias) {
- if (WasMustAlias)
- AST.TotalMayAliasSetSize += size();
- if (AS.Alias == SetMustAlias)
- AST.TotalMayAliasSetSize += AS.size();
+ // Merge the list of constituent pointers...
+ if (MemoryLocs.empty()) {
+ std::swap(MemoryLocs, AS.MemoryLocs);
+ } else {
+ llvm::append_range(MemoryLocs, AS.MemoryLocs);
+ AS.MemoryLocs.clear();
}
bool ASHadUnknownInsts = !AS.UnknownInsts.empty();
@@ -86,18 +87,6 @@ void AliasSet::mergeSetIn(AliasSet &AS, AliasSetTracker &AST,
AS.Forward = this; // Forward across AS now...
addRef(); // AS is now pointing to us...
- // Merge the list of constituent pointers...
- if (AS.PtrList) {
- SetSize += AS.size();
- AS.SetSize = 0;
- *PtrListEnd = AS.PtrList;
- AS.PtrList->setPrevInList(PtrListEnd);
- PtrListEnd = AS.PtrListEnd;
-
- AS.PtrList = nullptr;
- AS.PtrListEnd = &AS.PtrList;
- assert(*AS.PtrListEnd == nullptr && "End of list is not null?");
- }
if (ASHadUnknownInsts)
AS.dropRef(AST);
}
@@ -106,9 +95,8 @@ void AliasSetTracker::removeAliasSet(AliasSet *AS) {
if (AliasSet *Fwd = AS->Forward) {
Fwd->dropRef(*this);
AS->Forward = nullptr;
- } else // Update TotalMayAliasSetSize only if not forwarding.
- if (AS->Alias == AliasSet::SetMayAlias)
- TotalMayAliasSetSize -= AS->size();
+ } else // Update TotalAliasSetSize only if not forwarding.
+ TotalAliasSetSize -= AS->size();
AliasSets.erase(AS);
// If we've removed the saturated alias set, set saturated marker back to
@@ -124,42 +112,20 @@ void AliasSet::removeFromTracker(AliasSetTracker &AST) {
AST.removeAliasSet(this);
}
-void AliasSet::addPointer(AliasSetTracker &AST, PointerRec &Entry,
- LocationSize Size, const AAMDNodes &AAInfo,
- bool KnownMustAlias, bool SkipSizeUpdate) {
- assert(!Entry.hasAliasSet() && "Entry already in set!");
-
+void AliasSet::addPointer(AliasSetTracker &AST, const MemoryLocation& MemLoc,
+ bool KnownMustAlias) {
// Check to see if we have to downgrade to _may_ alias.
- if (isMustAlias())
- if (PointerRec *P = getSomePointer()) {
- if (!KnownMustAlias) {
- BatchAAResults &AA = AST.getAliasAnalysis();
- AliasResult Result = AA.alias(
- MemoryLocation(P->getValue(), P->getSize(), P->getAAInfo()),
- MemoryLocation(Entry.getValue(), Size, AAInfo));
- if (Result != AliasResult::MustAlias) {
- Alias = SetMayAlias;
- AST.TotalMayAliasSetSize += size();
- }
- assert(Result != AliasResult::NoAlias && "Cannot be part of must set!");
- } else if (!SkipSizeUpdate)
- P->updateSizeAndAAInfo(Size, AAInfo);
- }
-
- Entry.setAliasSet(this);
- Entry.updateSizeAndAAInfo(Size, AAInfo);
+ if (isMustAlias() && !KnownMustAlias)
+ for (const MemoryLocation& ASMemLoc : MemoryLocs)
+ if (!AST.getAliasAnalysis().isMustAlias(ASMemLoc, MemLoc)) {
+ Alias = SetMayAlias;
+ break;
+ }
// Add it to the end of the list...
- ++SetSize;
- assert(*PtrListEnd == nullptr && "End of list is not null?");
- *PtrListEnd = &Entry;
- PtrListEnd = Entry.setPrevInList(PtrListEnd);
- assert(*PtrListEnd == nullptr && "End of list is not null?");
- // Entry points to alias set.
- addRef();
-
- if (Alias == SetMayAlias)
- AST.TotalMayAliasSetSize++;
+ MemoryLocs.push_back(MemLoc);
+
+ AST.TotalAliasSetSize++;
}
void AliasSet::addUnknownInst(Instruction *I, BatchAAResults &AA) {
@@ -187,41 +153,21 @@ void AliasSet::addUnknownInst(Instruction *I, BatchAAResults &AA) {
/// members in the set return the appropriate AliasResult. Otherwise return
/// NoAlias.
///
-AliasResult AliasSet::aliasesPointer(const Value *Ptr, LocationSize Size,
- const AAMDNodes &AAInfo,
- BatchAAResults &AA) const {
+AliasResult AliasSet::aliasesPointer(const MemoryLocation &MemLoc, BatchAAResults &AA) const {
if (AliasAny)
return AliasResult::MayAlias;
- if (Alias == SetMustAlias) {
- assert(UnknownInsts.empty() && "Illegal must alias set!");
-
- // If this is a set of MustAliases, only check to see if the pointer aliases
- // SOME value in the set.
- PointerRec *SomePtr = getSomePointer();
- assert(SomePtr && "Empty must-alias set??");
- return AA.alias(MemoryLocation(SomePtr->getValue(), SomePtr->getSize(),
- SomePtr->getAAInfo()),
- MemoryLocation(Ptr, Size, AAInfo));
- }
-
- // If this is a may-alias set, we have to check all of the pointers in the set
- // to be sure it doesn't alias the set...
- for (iterator I = begin(), E = end(); I != E; ++I) {
- AliasResult AR =
- AA.alias(MemoryLocation(Ptr, Size, AAInfo),
- MemoryLocation(I.getPointer(), I.getSize(), I.getAAInfo()));
+ // Check all of the pointers in the set...
+ for (const auto& ASMemLoc : MemoryLocs) {
+ AliasResult AR = AA.alias(MemLoc, ASMemLoc);
if (AR != AliasResult::NoAlias)
return AR;
}
// Check the unknown instructions...
- if (!UnknownInsts.empty()) {
- for (Instruction *Inst : UnknownInsts)
- if (isModOrRefSet(
- AA.getModRefInfo(Inst, MemoryLocation(Ptr, Size, AAInfo))))
- return AliasResult::MayAlias;
- }
+ for (Instruction *Inst : UnknownInsts)
+ if (isModOrRefSet(AA.getModRefInfo(Inst, MemLoc)))
+ return AliasResult::MayAlias;
return AliasResult::NoAlias;
}
@@ -246,9 +192,8 @@ ModRefInfo AliasSet::aliasesUnknownInst(const Instruction *Inst,
}
ModRefInfo MR = ModRefInfo::NoModRef;
- for (iterator I = begin(), E = end(); I != E; ++I) {
- MR |= AA.getModRefInfo(
- Inst, MemoryLocation(I.getPointer(), I.getSize(), I.getAAInfo()));
+ for (const auto& ASMemLoc: MemoryLocs) {
+ MR |= AA.getModRefInfo(Inst, ASMemLoc);
if (isModAndRefSet(MR))
return MR;
}
@@ -257,13 +202,7 @@ ModRefInfo AliasSet::aliasesUnknownInst(const Instruction *Inst,
}
void AliasSetTracker::clear() {
- // Delete all the PointerRec entries.
- for (auto &I : PointerMap)
- I.second->eraseFromList();
-
PointerMap.clear();
-
- // The alias sets should all be clear now.
AliasSets.clear();
}
@@ -271,9 +210,7 @@ void AliasSetTracker::clear() {
/// alias the pointer. Return the unified set, or nullptr if no set that aliases
/// the pointer was found. MustAliasAll is updated to true/false if the pointer
/// is found to MustAlias all the sets it merged.
-AliasSet *AliasSetTracker::mergeAliasSetsForPointer(const Value *Ptr,
- LocationSize Size,
- const AAMDNodes &AAInfo,
+AliasSet *AliasSetTracker::mergeAliasSetsForPointer(const MemoryLocation &MemLoc,
bool &MustAliasAll) {
AliasSet *FoundSet = nullptr;
MustAliasAll = true;
@@ -281,7 +218,7 @@ AliasSet *AliasSetTracker::mergeAliasSetsForPointer(const Value *Ptr,
if (AS.Forward)
continue;
- AliasResult AR = AS.aliasesPointer(Ptr, Size, AAInfo, AA);
+ AliasResult AR = AS.aliasesPointer(MemLoc, AA);
if (AR == AliasResult::NoAlias)
continue;
@@ -317,59 +254,45 @@ AliasSet *AliasSetTracker::findAliasSetForUnknownInst(Instruction *Inst) {
}
AliasSet &AliasSetTracker::getAliasSetFor(const MemoryLocation &MemLoc) {
+ // Check if this MemLoc is already registered
+ AliasSet *&AS = PointerMap[MemLoc];
+ if (AS) {
+ if (AS->Forward) {
+ // ...
<truncated>
</pre>
</details>
https://github.com/llvm/llvm-project/pull/65731
More information about the llvm-commits
mailing list