diff --git a/llvm/lib/Transforms/Utils/MemorySSA.cpp b/llvm/lib/Transforms/Utils/MemorySSA.cpp index 8a93f4f5b16d..7285e9a342e6 100644 --- a/llvm/lib/Transforms/Utils/MemorySSA.cpp +++ b/llvm/lib/Transforms/Utils/MemorySSA.cpp @@ -594,7 +594,7 @@ class ClobberWalker { /// /// If this returns None, NewPaused is a vector of searches that terminated /// at StopWhere. Otherwise, NewPaused is left in an unspecified state. - Optional + Optional getBlockingAccess(MemoryAccess *StopWhere, SmallVectorImpl &PausedSearches, SmallVectorImpl &NewPaused, @@ -633,11 +633,12 @@ class ClobberWalker { assert(Res.Result != StopWhere || Res.FromCache); // If this wasn't a cache hit, we hit a clobber when walking. That's a // failure. + TerminatedPath Term{Res.Result, PathIndex}; if (!Res.FromCache || !MSSA.dominates(Res.Result, StopWhere)) - return PathIndex; + return Term; // Otherwise, it's a valid thing to potentially optimize to. - Terminated.push_back({Res.Result, PathIndex}); + Terminated.push_back(Term); continue; } @@ -769,22 +770,21 @@ class ClobberWalker { // liveOnEntry, and we'll happily wait for that to disappear (read: never) // For the moment, this is fine, since we do basically nothing with // blocker info. - if (Optional Blocker = getBlockingAccess( + if (Optional Blocker = getBlockingAccess( Target, PausedSearches, NewPaused, TerminatedPaths)) { - MemoryAccess *BlockingAccess = Paths[*Blocker].Last; // Cache our work on the blocking node, since we know that's correct. - cacheDefPath(Paths[*Blocker], BlockingAccess); + cacheDefPath(Paths[Blocker->LastNode], Blocker->Clobber); // Find the node we started at. We can't search based on N->Last, since // we may have gone around a loop with a different MemoryLocation. - auto Iter = find_if(def_path(*Blocker), [&](const DefPath &N) { + auto Iter = find_if(def_path(Blocker->LastNode), [&](const DefPath &N) { return defPathIndex(N) < PriorPathsSize; }); assert(Iter != def_path_iterator()); DefPath &CurNode = *Iter; assert(CurNode.Last == Current); - CurNode.Blocker = BlockingAccess; + CurNode.Blocker = Blocker->Clobber; // Two things: // A. We can't reliably cache all of NewPaused back. Consider a case diff --git a/llvm/unittests/Transforms/Utils/MemorySSA.cpp b/llvm/unittests/Transforms/Utils/MemorySSA.cpp index 93bda62f75ee..16264b68669f 100644 --- a/llvm/unittests/Transforms/Utils/MemorySSA.cpp +++ b/llvm/unittests/Transforms/Utils/MemorySSA.cpp @@ -344,3 +344,77 @@ TEST_F(MemorySSATest, TestStoreDoubleQuery) { EXPECT_EQ(Clobber, StoreAccess); EXPECT_TRUE(MSSA.isLiveOnEntryDef(LiveOnEntry)); } + +// Bug: During phi optimization, the walker wouldn't cache to the proper result +// in the farthest-walked BB. +// +// Specifically, it would assume that whatever we walked to was a clobber. +// "Whatever we walked to" isn't a clobber if we hit a cache entry. +// +// ...So, we need a test case that looks like: +// A +// / \ +// B | +// \ / +// C +// +// Where, when we try to optimize a thing in 'C', a blocker is found in 'B'. +// The walk must determine that the blocker exists by using cache entries *while +// walking* 'B'. +TEST_F(MemorySSATest, PartialWalkerCacheWithPhis) { + F = Function::Create(FunctionType::get(B.getVoidTy(), {}, false), + GlobalValue::ExternalLinkage, "F", &M); + B.SetInsertPoint(BasicBlock::Create(C, "A", F)); + Type *Int8 = Type::getInt8Ty(C); + Constant *One = ConstantInt::get(Int8, 1); + Constant *Zero = ConstantInt::get(Int8, 0); + Value *AllocA = B.CreateAlloca(Int8, One, "a"); + Value *AllocB = B.CreateAlloca(Int8, One, "b"); + BasicBlock *IfThen = BasicBlock::Create(C, "B", F); + BasicBlock *IfEnd = BasicBlock::Create(C, "C", F); + + B.CreateCondBr(UndefValue::get(Type::getInt1Ty(C)), IfThen, IfEnd); + + B.SetInsertPoint(IfThen); + Instruction *FirstStore = B.CreateStore(Zero, AllocA); + B.CreateStore(Zero, AllocB); + Instruction *ALoad0 = B.CreateLoad(AllocA, ""); + Instruction *BStore = B.CreateStore(Zero, AllocB); + // Due to use optimization/etc. we make a store to A, which is removed after + // we build MSSA. This helps keep the test case simple-ish. + Instruction *KillStore = B.CreateStore(Zero, AllocA); + Instruction *ALoad = B.CreateLoad(AllocA, ""); + B.CreateBr(IfEnd); + + B.SetInsertPoint(IfEnd); + Instruction *BelowPhi = B.CreateStore(Zero, AllocA); + + setupAnalyses(); + MemorySSA &MSSA = Analyses->MSSA; + MemorySSAWalker *Walker = Analyses->Walker; + + // Kill `KillStore`; it exists solely so that the load after it won't be + // optimized to FirstStore. + MSSA.removeMemoryAccess(MSSA.getMemoryAccess(KillStore)); + KillStore->eraseFromParent(); + auto *ALoadMA = cast(MSSA.getMemoryAccess(ALoad)); + EXPECT_EQ(ALoadMA->getDefiningAccess(), MSSA.getMemoryAccess(BStore)); + + // Populate the cache for the store to AllocB directly after FirstStore. It + // should point to something in block B (so something in D can't be optimized + // to it). + MemoryAccess *Load0Clobber = Walker->getClobberingMemoryAccess(ALoad0); + EXPECT_EQ(MSSA.getMemoryAccess(FirstStore), Load0Clobber); + + // If the bug exists, this will introduce a bad cache entry for %a on BStore. + // It will point to the store to %b after FirstStore. This only happens during + // phi optimization. + MemoryAccess *BottomClobber = Walker->getClobberingMemoryAccess(BelowPhi); + MemoryAccess *Phi = MSSA.getMemoryAccess(IfEnd); + EXPECT_EQ(BottomClobber, Phi); + + // This query will first check the cache for {%a, BStore}. It should point to + // FirstStore, not to the store after FirstStore. + MemoryAccess *UseClobber = Walker->getClobberingMemoryAccess(ALoad); + EXPECT_EQ(UseClobber, MSSA.getMemoryAccess(FirstStore)); +}