[llvm] 5a63813 - [DependenceAnalysis] Dependecies for loads marked with "ivnariant.load" should not be shared with general accesses(PR42151).
Evgeniy Brevnov via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 4 03:40:46 PST 2020
Author: Evgeniy Brevnov
Date: 2020-03-04T18:40:02+07:00
New Revision: 5a63813dc7fc4e3cd0c0b7b71bef031a69c9385f
URL: https://github.com/llvm/llvm-project/commit/5a63813dc7fc4e3cd0c0b7b71bef031a69c9385f
DIFF: https://github.com/llvm/llvm-project/commit/5a63813dc7fc4e3cd0c0b7b71bef031a69c9385f.diff
LOG: [DependenceAnalysis] Dependecies for loads marked with "ivnariant.load" should not be shared with general accesses(PR42151).
Summary:
This is second attempt to fix the problem with incorrect dependencies reported in presence of invariant load. Initial fix (https://reviews.llvm.org/D64405) was reverted due to a regression reported in https://reviews.llvm.org/D70516.
The original fix changed caching behavior for invariant loads. Namely such loads are not put into the second level cache (NonLocalDepInfo). The problem with that fix is the first level cache (CachedNonLocalPointerInfo) still works as if invariant loads were in the second level cache. The solution is in addition to not putting dependence results into the second level cache avoid putting info about invariant loads into the first level cache as well.
Reviewers: jdoerfert, reames, hfinkel, efriedma
Reviewed By: jdoerfert
Subscribers: DaniilSuchkov, hiraditya, bmahjour, llvm-commits
Tags: #llvm
Differential Revision: https://reviews.llvm.org/D73027
Added:
Modified:
llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
Removed:
################################################################################
diff --git a/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp b/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
index dacc5a87a337..142c7fe30523 100644
--- a/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
+++ b/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
@@ -889,6 +889,11 @@ MemDepResult MemoryDependenceResults::GetNonLocalInfoForBlock(
Instruction *QueryInst, const MemoryLocation &Loc, bool isLoad,
BasicBlock *BB, NonLocalDepInfo *Cache, unsigned NumSortedEntries) {
+ bool isInvariantLoad = false;
+
+ if (LoadInst *LI = dyn_cast_or_null<LoadInst>(QueryInst))
+ isInvariantLoad = LI->getMetadata(LLVMContext::MD_invariant_load);
+
// Do a binary search to see if we already have an entry for this block in
// the cache set. If so, find it.
NonLocalDepInfo::iterator Entry = std::upper_bound(
@@ -900,6 +905,13 @@ MemDepResult MemoryDependenceResults::GetNonLocalInfoForBlock(
if (Entry != Cache->begin() + NumSortedEntries && Entry->getBB() == BB)
ExistingResult = &*Entry;
+ // Use cached result for invariant load only if there is no dependency for non
+ // invariant load. In this case invariant load can not have any dependency as
+ // well.
+ if (ExistingResult && isInvariantLoad &&
+ !ExistingResult->getResult().isNonFuncLocal())
+ ExistingResult = nullptr;
+
// If we have a cached entry, and it is non-dirty, use it as the value for
// this dependency.
if (ExistingResult && !ExistingResult->getResult().isDirty()) {
@@ -928,6 +940,10 @@ MemDepResult MemoryDependenceResults::GetNonLocalInfoForBlock(
MemDepResult Dep =
getPointerDependencyFrom(Loc, isLoad, ScanPos, BB, QueryInst);
+ // Don't cache results for invariant load.
+ if (isInvariantLoad)
+ return Dep;
+
// If we had a dirty entry for the block, update it. Otherwise, just add
// a new entry.
if (ExistingResult)
@@ -1017,6 +1033,10 @@ bool MemoryDependenceResults::getNonLocalPointerDepFromBB(
InitialNLPI.Size = Loc.Size;
InitialNLPI.AATags = Loc.AATags;
+ bool isInvariantLoad = false;
+ if (LoadInst *LI = dyn_cast_or_null<LoadInst>(QueryInst))
+ isInvariantLoad = LI->getMetadata(LLVMContext::MD_invariant_load);
+
// Get the NLPI for CacheKey, inserting one into the map if it doesn't
// already have one.
std::pair<CachedNonLocalPointerInfo::iterator, bool> Pair =
@@ -1025,7 +1045,8 @@ bool MemoryDependenceResults::getNonLocalPointerDepFromBB(
// If we already have a cache entry for this CacheKey, we may need to do some
// work to reconcile the cache entry and the current query.
- if (!Pair.second) {
+ // Invariant loads don't participate in caching. Thus no need to reconcile.
+ if (!isInvariantLoad && !Pair.second) {
if (CacheInfo->Size != Loc.Size) {
bool ThrowOutEverything;
if (CacheInfo->Size.hasValue() && Loc.Size.hasValue()) {
@@ -1092,7 +1113,9 @@ bool MemoryDependenceResults::getNonLocalPointerDepFromBB(
// Don't use cached information for invariant loads since it is valid for
// non-invariant loads only.
//
- if (!IsIncomplete &&
+ // Don't use cached information for invariant loads since it is valid for
+ // non-invariant loads only.
+ if (!IsIncomplete && !isInvariantLoad &&
CacheInfo->Pair == BBSkipFirstBlockPair(StartBB, SkipFirstBlock)) {
// We have a fully cached result for this query then we can just return the
// cached results and populate the visited set. However, we have to verify
@@ -1133,10 +1156,15 @@ bool MemoryDependenceResults::getNonLocalPointerDepFromBB(
// pointer or one that we're about to invalidate by putting more info into
// it than its valid cache info. If empty and not explicitly indicated as
// incomplete, the result will be valid cache info, otherwise it isn't.
- if (!IsIncomplete && Cache->empty())
- CacheInfo->Pair = BBSkipFirstBlockPair(StartBB, SkipFirstBlock);
- else
- CacheInfo->Pair = BBSkipFirstBlockPair();
+ //
+ // Invariant loads don't affect cache in any way thus no need to update
+ // CacheInfo as well.
+ if (!isInvariantLoad) {
+ if (!IsIncomplete && Cache->empty())
+ CacheInfo->Pair = BBSkipFirstBlockPair(StartBB, SkipFirstBlock);
+ else
+ CacheInfo->Pair = BBSkipFirstBlockPair();
+ }
SmallVector<BasicBlock *, 32> Worklist;
Worklist.push_back(StartBB);
@@ -1377,22 +1405,27 @@ bool MemoryDependenceResults::getNonLocalPointerDepFromBB(
if (SkipFirstBlock)
return false;
- bool foundBlock = false;
- for (NonLocalDepEntry &I : llvm::reverse(*Cache)) {
- if (I.getBB() != BB)
- continue;
+ // Results of invariant loads are not cached thus no need to update cached
+ // information.
+ if (!isInvariantLoad) {
+ for (NonLocalDepEntry &I : llvm::reverse(*Cache)) {
+ if (I.getBB() != BB)
+ continue;
- assert((GotWorklistLimit || I.getResult().isNonLocal() ||
- !DT.isReachableFromEntry(BB)) &&
- "Should only be here with transparent block");
- foundBlock = true;
- I.setResult(MemDepResult::getUnknown());
- Result.push_back(
- NonLocalDepResult(I.getBB(), I.getResult(), Pointer.getAddr()));
- break;
+ assert((GotWorklistLimit || I.getResult().isNonLocal() ||
+ !DT.isReachableFromEntry(BB)) &&
+ "Should only be here with transparent block");
+
+ I.setResult(MemDepResult::getUnknown());
+
+
+ break;
+ }
}
- (void)foundBlock; (void)GotWorklistLimit;
- assert((foundBlock || GotWorklistLimit) && "Current block not in cache?");
+ (void)GotWorklistLimit;
+ // Go ahead and report unknown dependence.
+ Result.push_back(
+ NonLocalDepResult(BB, MemDepResult::getUnknown(), Pointer.getAddr()));
}
// Okay, we're done now. If we added new values to the cache, re-sort it.
More information about the llvm-commits
mailing list