[MSSA] Fix a caching bug.
This fixes a bug where we'd sometimes cache overly-conservative results with our walker. This bug was made more obvious by r277480, which makes our cache far more spotty than it was. Test case is llvm-unit, because we're likely going to use CachingWalker only for def optimization in the future. The bug stems from that there was a place where the walker assumed that `DefNode.Last` was a valid target to cache to when failing to optimize phis. This is sometimes incorrect if we have a cache hit. The fix is to use the thing we *can* assume is a valid target to cache to. :) llvm-svn: 277559
This commit is contained in:
parent
9f0ef01197
commit
14633b5cd3
|
@ -594,7 +594,7 @@ class ClobberWalker {
|
||||||
///
|
///
|
||||||
/// If this returns None, NewPaused is a vector of searches that terminated
|
/// If this returns None, NewPaused is a vector of searches that terminated
|
||||||
/// at StopWhere. Otherwise, NewPaused is left in an unspecified state.
|
/// at StopWhere. Otherwise, NewPaused is left in an unspecified state.
|
||||||
Optional<ListIndex>
|
Optional<TerminatedPath>
|
||||||
getBlockingAccess(MemoryAccess *StopWhere,
|
getBlockingAccess(MemoryAccess *StopWhere,
|
||||||
SmallVectorImpl<ListIndex> &PausedSearches,
|
SmallVectorImpl<ListIndex> &PausedSearches,
|
||||||
SmallVectorImpl<ListIndex> &NewPaused,
|
SmallVectorImpl<ListIndex> &NewPaused,
|
||||||
|
@ -633,11 +633,12 @@ class ClobberWalker {
|
||||||
assert(Res.Result != StopWhere || Res.FromCache);
|
assert(Res.Result != StopWhere || Res.FromCache);
|
||||||
// If this wasn't a cache hit, we hit a clobber when walking. That's a
|
// If this wasn't a cache hit, we hit a clobber when walking. That's a
|
||||||
// failure.
|
// failure.
|
||||||
|
TerminatedPath Term{Res.Result, PathIndex};
|
||||||
if (!Res.FromCache || !MSSA.dominates(Res.Result, StopWhere))
|
if (!Res.FromCache || !MSSA.dominates(Res.Result, StopWhere))
|
||||||
return PathIndex;
|
return Term;
|
||||||
|
|
||||||
// Otherwise, it's a valid thing to potentially optimize to.
|
// Otherwise, it's a valid thing to potentially optimize to.
|
||||||
Terminated.push_back({Res.Result, PathIndex});
|
Terminated.push_back(Term);
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -769,22 +770,21 @@ class ClobberWalker {
|
||||||
// liveOnEntry, and we'll happily wait for that to disappear (read: never)
|
// liveOnEntry, and we'll happily wait for that to disappear (read: never)
|
||||||
// For the moment, this is fine, since we do basically nothing with
|
// For the moment, this is fine, since we do basically nothing with
|
||||||
// blocker info.
|
// blocker info.
|
||||||
if (Optional<ListIndex> Blocker = getBlockingAccess(
|
if (Optional<TerminatedPath> Blocker = getBlockingAccess(
|
||||||
Target, PausedSearches, NewPaused, TerminatedPaths)) {
|
Target, PausedSearches, NewPaused, TerminatedPaths)) {
|
||||||
MemoryAccess *BlockingAccess = Paths[*Blocker].Last;
|
|
||||||
// Cache our work on the blocking node, since we know that's correct.
|
// 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
|
// 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.
|
// 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;
|
return defPathIndex(N) < PriorPathsSize;
|
||||||
});
|
});
|
||||||
assert(Iter != def_path_iterator());
|
assert(Iter != def_path_iterator());
|
||||||
|
|
||||||
DefPath &CurNode = *Iter;
|
DefPath &CurNode = *Iter;
|
||||||
assert(CurNode.Last == Current);
|
assert(CurNode.Last == Current);
|
||||||
CurNode.Blocker = BlockingAccess;
|
CurNode.Blocker = Blocker->Clobber;
|
||||||
|
|
||||||
// Two things:
|
// Two things:
|
||||||
// A. We can't reliably cache all of NewPaused back. Consider a case
|
// A. We can't reliably cache all of NewPaused back. Consider a case
|
||||||
|
|
|
@ -344,3 +344,77 @@ TEST_F(MemorySSATest, TestStoreDoubleQuery) {
|
||||||
EXPECT_EQ(Clobber, StoreAccess);
|
EXPECT_EQ(Clobber, StoreAccess);
|
||||||
EXPECT_TRUE(MSSA.isLiveOnEntryDef(LiveOnEntry));
|
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<MemoryUse>(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));
|
||||||
|
}
|
||||||
|
|
Loading…
Reference in New Issue