[llvm] 17994ed - [MemorySSA] Remove PerformedPhiTranslation flag
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 21 01:32:22 PDT 2022
Author: Nikita Popov
Date: 2022-09-21T10:32:09+02:00
New Revision: 17994ed91980b9964a6a9d0bf5a0f7b3b9deec97
URL: https://github.com/llvm/llvm-project/commit/17994ed91980b9964a6a9d0bf5a0f7b3b9deec97
DIFF: https://github.com/llvm/llvm-project/commit/17994ed91980b9964a6a9d0bf5a0f7b3b9deec97.diff
LOG: [MemorySSA] Remove PerformedPhiTranslation flag
I believe this is no longer necessary, as the underlying problem
has been fixed in a different way: Nowadays, we will adjust the
location size to beforeOrAfterPointer() if the pointer is not loop
invariant. This makes merging results translated across loop
backedges safe.
The two tests in phi-translation.ll show an improvement while still
being correct: The loads in the loop no longer alias with noalias
pointers, but still alias with the store in the entry block (which
they originally did not -- this is the bug that
PerformedPhiTranslation originally fixed).
Differential Revision: https://reviews.llvm.org/D133404
Added:
Modified:
llvm/include/llvm/Analysis/MemorySSA.h
llvm/lib/Analysis/MemorySSA.cpp
llvm/test/Analysis/MemorySSA/phi-translation.ll
Removed:
################################################################################
diff --git a/llvm/include/llvm/Analysis/MemorySSA.h b/llvm/include/llvm/Analysis/MemorySSA.h
index 4a5eeb5a40201..caf4afd431be6 100644
--- a/llvm/include/llvm/Analysis/MemorySSA.h
+++ b/llvm/include/llvm/Analysis/MemorySSA.h
@@ -1202,11 +1202,9 @@ class upward_defs_iterator
using BaseT = upward_defs_iterator::iterator_facade_base;
public:
- upward_defs_iterator(const MemoryAccessPair &Info, DominatorTree *DT,
- bool *PerformedPhiTranslation = nullptr)
+ upward_defs_iterator(const MemoryAccessPair &Info, DominatorTree *DT)
: DefIterator(Info.first), Location(Info.second),
- OriginalAccess(Info.first), DT(DT),
- PerformedPhiTranslation(PerformedPhiTranslation) {
+ OriginalAccess(Info.first), DT(DT) {
CurrentPair.first = nullptr;
WalkingPhi = Info.first && isa<MemoryPhi>(Info.first);
@@ -1270,9 +1268,6 @@ class upward_defs_iterator
!IsGuaranteedLoopInvariant(TransAddr))
CurrentPair.second = CurrentPair.second.getWithNewSize(
LocationSize::beforeOrAfterPointer());
-
- if (PerformedPhiTranslation)
- *PerformedPhiTranslation = true;
}
}
}
@@ -1284,13 +1279,11 @@ class upward_defs_iterator
MemoryAccess *OriginalAccess = nullptr;
DominatorTree *DT = nullptr;
bool WalkingPhi = false;
- bool *PerformedPhiTranslation = nullptr;
};
inline upward_defs_iterator
-upward_defs_begin(const MemoryAccessPair &Pair, DominatorTree &DT,
- bool *PerformedPhiTranslation = nullptr) {
- return upward_defs_iterator(Pair, &DT, PerformedPhiTranslation);
+upward_defs_begin(const MemoryAccessPair &Pair, DominatorTree &DT) {
+ return upward_defs_iterator(Pair, &DT);
}
inline upward_defs_iterator upward_defs_end() { return upward_defs_iterator(); }
diff --git a/llvm/lib/Analysis/MemorySSA.cpp b/llvm/lib/Analysis/MemorySSA.cpp
index ece45e528d10c..461886650261b 100644
--- a/llvm/lib/Analysis/MemorySSA.cpp
+++ b/llvm/lib/Analysis/MemorySSA.cpp
@@ -528,10 +528,6 @@ template <class AliasAnalysisType> class ClobberWalker {
// List of visited <Access, Location> pairs; we can skip paths already
// visited with the same memory location.
DenseSet<ConstMemoryAccessPair> VisitedPhis;
- // Record if phi translation has been performed during the current phi
- // optimization walk, as merging alias results after phi translation can
- // yield incorrect results. Context in PR46156.
- bool PerformedPhiTranslation = false;
/// Find the nearest def or phi that `From` can legally be optimized to.
const MemoryAccess *getWalkTarget(const MemoryPhi *From) const {
@@ -603,8 +599,7 @@ template <class AliasAnalysisType> class ClobberWalker {
void addSearches(MemoryPhi *Phi, SmallVectorImpl<ListIndex> &PausedSearches,
ListIndex PriorNode) {
- auto UpwardDefsBegin = upward_defs_begin({Phi, Paths[PriorNode].Loc}, DT,
- &PerformedPhiTranslation);
+ auto UpwardDefsBegin = upward_defs_begin({Phi, Paths[PriorNode].Loc}, DT);
auto UpwardDefs = make_range(UpwardDefsBegin, upward_defs_end());
for (const MemoryAccessPair &P : UpwardDefs) {
PausedSearches.push_back(Paths.size());
@@ -659,16 +654,8 @@ template <class AliasAnalysisType> class ClobberWalker {
// - We still cache things for A, so C only needs to walk up a bit.
// If this behavior becomes problematic, we can fix without a ton of extra
// work.
- if (!VisitedPhis.insert({Node.Last, Node.Loc}).second) {
- if (PerformedPhiTranslation) {
- // If visiting this path performed Phi translation, don't continue,
- // since it may not be correct to merge results from two paths if one
- // relies on the phi translation.
- TerminatedPath Term{Node.Last, PathIndex};
- return Term;
- }
+ if (!VisitedPhis.insert({Node.Last, Node.Loc}).second)
continue;
- }
const MemoryAccess *SkipStopWhere = nullptr;
if (Query->SkipSelfAccess && Node.Loc == Query->StartingLoc) {
@@ -781,7 +768,7 @@ template <class AliasAnalysisType> class ClobberWalker {
/// terminates when a MemoryAccess that clobbers said MemoryLocation is found.
OptznResult tryOptimizePhi(MemoryPhi *Phi, MemoryAccess *Start,
const MemoryLocation &Loc) {
- assert(Paths.empty() && VisitedPhis.empty() && !PerformedPhiTranslation &&
+ assert(Paths.empty() && VisitedPhis.empty() &&
"Reset the optimization state.");
Paths.emplace_back(Loc, Start, Phi, None);
@@ -937,7 +924,6 @@ template <class AliasAnalysisType> class ClobberWalker {
void resetPhiOptznState() {
Paths.clear();
VisitedPhis.clear();
- PerformedPhiTranslation = false;
}
public:
diff --git a/llvm/test/Analysis/MemorySSA/phi-translation.ll b/llvm/test/Analysis/MemorySSA/phi-translation.ll
index fc79db91be362..d484cb1cde40d 100644
--- a/llvm/test/Analysis/MemorySSA/phi-translation.ll
+++ b/llvm/test/Analysis/MemorySSA/phi-translation.ll
@@ -296,7 +296,8 @@ define i32 @dont_merge_noalias_simple(i32* noalias %ptr) {
; CHECK-NEXT: store i16 1, i16* %s1.ptr, align 2
; CHECK-LABEL: %for.body
-; CHECK: ; MemoryUse(4)
+; NOLIMIT: ; MemoryUse(1)
+; LIMIT: ; MemoryUse(4)
; CHECK-NEXT: %lv = load i16, i16* %arrayidx, align 2
entry:
@@ -329,7 +330,8 @@ define i32 @dont_merge_noalias_complex(i32* noalias %ptr, i32* noalias %another)
; CHECK-NEXT: store i16 1, i16* %s1.ptr, align 2
; CHECK-LABEL: %for.body
-; CHECK: ; MemoryUse(7)
+; NOLIMIT: ; MemoryUse(1)
+; LIMIT: ; MemoryUse(7)
; CHECK-NEXT: %lv = load i16, i16* %arrayidx, align 2
entry:
More information about the llvm-commits
mailing list