From 17994ed91980b9964a6a9d0bf5a0f7b3b9deec97 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Wed, 7 Sep 2022 10:20:20 +0200 Subject: [PATCH] [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 --- llvm/include/llvm/Analysis/MemorySSA.h | 15 ++++----------- llvm/lib/Analysis/MemorySSA.cpp | 20 +++----------------- llvm/test/Analysis/MemorySSA/phi-translation.ll | 6 ++++-- 3 files changed, 11 insertions(+), 30 deletions(-) diff --git a/llvm/include/llvm/Analysis/MemorySSA.h b/llvm/include/llvm/Analysis/MemorySSA.h index 4a5eeb5..caf4afd 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(Info.first); @@ -1270,9 +1268,6 @@ private: !IsGuaranteedLoopInvariant(TransAddr)) CurrentPair.second = CurrentPair.second.getWithNewSize( LocationSize::beforeOrAfterPointer()); - - if (PerformedPhiTranslation) - *PerformedPhiTranslation = true; } } } @@ -1284,13 +1279,11 @@ private: 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 ece45e5..46188665 100644 --- a/llvm/lib/Analysis/MemorySSA.cpp +++ b/llvm/lib/Analysis/MemorySSA.cpp @@ -528,10 +528,6 @@ template class ClobberWalker { // List of visited pairs; we can skip paths already // visited with the same memory location. DenseSet 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 ClobberWalker { void addSearches(MemoryPhi *Phi, SmallVectorImpl &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 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 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 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 fc79db9..d484cb1 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: -- 2.7.4