[llvm] 21fbd55 - Reapply [LVI] Normalize pointer behavior
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 13 00:08:08 PST 2019
Author: Nikita Popov
Date: 2019-12-13T08:59:58+01:00
New Revision: 21fbd5587cdfa11dabb3aeb0ead2d3d5fd0b490d
URL: https://github.com/llvm/llvm-project/commit/21fbd5587cdfa11dabb3aeb0ead2d3d5fd0b490d
DIFF: https://github.com/llvm/llvm-project/commit/21fbd5587cdfa11dabb3aeb0ead2d3d5fd0b490d.diff
LOG: Reapply [LVI] Normalize pointer behavior
This is a rebase of the change over D70376, which fixes an LVI cache
invalidation issue that also affected this patch.
-----
Related to D69686. As noted there, LVI currently behaves differently
for integer and pointer values: For integers, the block value is always
valid inside the basic block, while for pointers it is only valid at
the end of the basic block. I believe the integer behavior is the
correct one, and CVP relies on it via its getConstantRange() uses.
The reason for the special pointer behavior is that LVI checks whether
a pointer is dereferenced in a given basic block and marks it as
non-null in that case. Of course, this information is valid only after
the dereferencing instruction, or in conservative approximation,
at the end of the block.
This patch changes the treatment of dereferencability: Instead of
including it inside the block value, we instead treat it as something
similar to an assume (it essentially is a non-nullness assume) and
incorporate this information in intersectAssumeOrGuardBlockValueConstantRange()
if the context instruction is the terminator of the basic block.
This happens either when determining an edge-value internally in LVI,
or when a terminator was explicitly passed to getValueAt(). The latter
case makes this change not fully NFC, because we can now fold
terminator icmps based on the dereferencability information in the
same block. This is the reason why I changed one JumpThreading test
(it would optimize the condition away without the change).
Of course, we do not want to recompute dereferencability on each
intersectAssume call, so we need a new cache for this. The
dereferencability analysis requires walking the entire basic block
and computing underlying objects of all memory operands. This was
previously done separately for each queried pointer value. In the
new implementation (both because this makes the caching simpler,
and because it is faster), I instead only walk the full BB once and
cache all the dereferenced pointers. So the traversal is now performed
only once per BB, instead of once per queried pointer value.
I think the overall model now makes more sense than before, and there
will be no more pitfalls due to differing integer/pointer behavior.
Differential Revision: https://reviews.llvm.org/D69914
Added:
Modified:
llvm/lib/Analysis/LazyValueInfo.cpp
llvm/test/Transforms/JumpThreading/combine-metadata.ll
Removed:
################################################################################
diff --git a/llvm/lib/Analysis/LazyValueInfo.cpp b/llvm/lib/Analysis/LazyValueInfo.cpp
index 78ad5859de4c..4a05801c17ee 100644
--- a/llvm/lib/Analysis/LazyValueInfo.cpp
+++ b/llvm/lib/Analysis/LazyValueInfo.cpp
@@ -154,10 +154,14 @@ namespace {
class LazyValueInfoCache {
/// This is all of the cached information for one basic block. It contains
/// the per-value lattice elements, as well as a separate set for
- /// overdefined values to reduce memory usage.
+ /// overdefined values to reduce memory usage. Additionally pointers
+ /// dereferenced in the block are cached for nullability queries.
struct BlockCacheEntryTy {
SmallDenseMap<AssertingVH<Value>, ValueLatticeElement, 4> LatticeElements;
SmallDenseSet<AssertingVH<Value>, 4> OverDefined;
+ // None indicates that the dereferenced pointers for this basic block
+ // block have not been computed yet.
+ Optional<DenseSet<AssertingVH<Value>>> DereferencedPointers;
};
/// Cached information per basic block.
@@ -205,6 +209,22 @@ namespace {
return LatticeIt->second;
}
+ bool isPointerDereferencedInBlock(
+ Value *V, BasicBlock *BB,
+ std::function<DenseSet<AssertingVH<Value>>(BasicBlock *)> InitFn) {
+ auto &CacheEntry = BlockCache.try_emplace(BB).first->second;
+ if (!CacheEntry.DereferencedPointers) {
+ CacheEntry.DereferencedPointers = InitFn(BB);
+ for (Value *V : *CacheEntry.DereferencedPointers) {
+ auto HandleIt = ValueHandles.find_as(V);
+ if (HandleIt == ValueHandles.end())
+ ValueHandles.insert({ V, this });
+ }
+ }
+
+ return CacheEntry.DereferencedPointers->count(V);
+ }
+
/// clear - Empty the cache.
void clear() {
BlockCache.clear();
@@ -229,6 +249,8 @@ void LazyValueInfoCache::eraseValue(Value *V) {
for (auto &Pair : BlockCache) {
Pair.second.LatticeElements.erase(V);
Pair.second.OverDefined.erase(V);
+ if (Pair.second.DereferencedPointers)
+ Pair.second.DereferencedPointers->erase(V);
}
auto HandleIt = ValueHandles.find_as(V);
@@ -393,6 +415,7 @@ namespace {
BasicBlock *BB);
bool solveBlockValueExtractValue(ValueLatticeElement &BBLV,
ExtractValueInst *EVI, BasicBlock *BB);
+ bool isNonNullDueToDereferenceInBlock(Value *Val, BasicBlock *BB);
void intersectAssumeOrGuardBlockValueConstantRange(Value *Val,
ValueLatticeElement &BBLV,
Instruction *BBI);
@@ -574,17 +597,6 @@ bool LazyValueInfoImpl::solveBlockValue(Value *Val, BasicBlock *BB) {
bool LazyValueInfoImpl::solveBlockValueImpl(ValueLatticeElement &Res,
Value *Val, BasicBlock *BB) {
-
- Instruction *BBI = dyn_cast<Instruction>(Val);
- if (!BBI || BBI->getParent() != BB)
- return solveBlockValueNonLocal(Res, Val, BB);
-
- if (PHINode *PN = dyn_cast<PHINode>(BBI))
- return solveBlockValuePHINode(Res, PN, BB);
-
- if (auto *SI = dyn_cast<SelectInst>(BBI))
- return solveBlockValueSelect(Res, SI, BB);
-
// If this value is a nonnull pointer, record it's range and bailout. Note
// that for all other pointer typed values, we terminate the search at the
// definition. We could easily extend this to look through geps, bitcasts,
@@ -594,11 +606,22 @@ bool LazyValueInfoImpl::solveBlockValueImpl(ValueLatticeElement &Res,
// This does mean that we have a sensitivity to where the defining
// instruction is placed, even if it could legally be hoisted much higher.
// That is unfortunate.
- PointerType *PT = dyn_cast<PointerType>(BBI->getType());
- if (PT && isKnownNonZero(BBI, DL)) {
+ PointerType *PT = dyn_cast<PointerType>(Val->getType());
+ if (PT && isKnownNonZero(Val, DL)) {
Res = ValueLatticeElement::getNot(ConstantPointerNull::get(PT));
return true;
}
+
+ Instruction *BBI = dyn_cast<Instruction>(Val);
+ if (!BBI || BBI->getParent() != BB)
+ return solveBlockValueNonLocal(Res, Val, BB);
+
+ if (PHINode *PN = dyn_cast<PHINode>(BBI))
+ return solveBlockValuePHINode(Res, PN, BB);
+
+ if (auto *SI = dyn_cast<SelectInst>(BBI))
+ return solveBlockValueSelect(Res, SI, BB);
+
if (BBI->getType()->isIntegerTy()) {
if (auto *CI = dyn_cast<CastInst>(BBI))
return solveBlockValueCast(Res, CI, BB);
@@ -619,75 +642,61 @@ bool LazyValueInfoImpl::solveBlockValueImpl(ValueLatticeElement &Res,
return true;
}
-static bool InstructionDereferencesPointer(Instruction *I, Value *Ptr) {
- if (LoadInst *L = dyn_cast<LoadInst>(I)) {
- return L->getPointerAddressSpace() == 0 &&
- GetUnderlyingObject(L->getPointerOperand(),
- L->getModule()->getDataLayout()) == Ptr;
- }
- if (StoreInst *S = dyn_cast<StoreInst>(I)) {
- return S->getPointerAddressSpace() == 0 &&
- GetUnderlyingObject(S->getPointerOperand(),
- S->getModule()->getDataLayout()) == Ptr;
+static void AddDereferencedPointer(
+ Value *Ptr, DenseSet<AssertingVH<Value>> &PtrSet, const DataLayout &DL) {
+ // TODO: Use NullPointerIsDefined instead.
+ if (Ptr->getType()->getPointerAddressSpace() == 0) {
+ Ptr = GetUnderlyingObject(Ptr, DL);
+ PtrSet.insert(Ptr);
}
- if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(I)) {
- if (MI->isVolatile()) return false;
+}
+
+static void AddPointersDereferencedByInstruction(
+ Instruction *I, DenseSet<AssertingVH<Value>> &PtrSet,
+ const DataLayout &DL) {
+ if (LoadInst *L = dyn_cast<LoadInst>(I)) {
+ AddDereferencedPointer(L->getPointerOperand(), PtrSet, DL);
+ } else if (StoreInst *S = dyn_cast<StoreInst>(I)) {
+ AddDereferencedPointer(S->getPointerOperand(), PtrSet, DL);
+ } else if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(I)) {
+ if (MI->isVolatile()) return;
// FIXME: check whether it has a valuerange that excludes zero?
ConstantInt *Len = dyn_cast<ConstantInt>(MI->getLength());
- if (!Len || Len->isZero()) return false;
+ if (!Len || Len->isZero()) return;
- if (MI->getDestAddressSpace() == 0)
- if (GetUnderlyingObject(MI->getRawDest(),
- MI->getModule()->getDataLayout()) == Ptr)
- return true;
+ AddDereferencedPointer(MI->getRawDest(), PtrSet, DL);
if (MemTransferInst *MTI = dyn_cast<MemTransferInst>(MI))
- if (MTI->getSourceAddressSpace() == 0)
- if (GetUnderlyingObject(MTI->getRawSource(),
- MTI->getModule()->getDataLayout()) == Ptr)
- return true;
+ AddDereferencedPointer(MTI->getRawSource(), PtrSet, DL);
}
- return false;
}
-/// Return true if the allocation associated with Val is ever dereferenced
-/// within the given basic block. This establishes the fact Val is not null,
-/// but does not imply that the memory at Val is dereferenceable. (Val may
-/// point off the end of the dereferenceable part of the object.)
-static bool isObjectDereferencedInBlock(Value *Val, BasicBlock *BB) {
- assert(Val->getType()->isPointerTy());
+bool LazyValueInfoImpl::isNonNullDueToDereferenceInBlock(
+ Value *Val, BasicBlock *BB) {
+ if (NullPointerIsDefined(BB->getParent(),
+ Val->getType()->getPointerAddressSpace()))
+ return false;
const DataLayout &DL = BB->getModule()->getDataLayout();
- Value *UnderlyingVal = GetUnderlyingObject(Val, DL);
- // If 'GetUnderlyingObject' didn't converge, skip it. It won't converge
- // inside InstructionDereferencesPointer either.
- if (UnderlyingVal == GetUnderlyingObject(UnderlyingVal, DL, 1))
+ Val = GetUnderlyingObject(Val, DL);
+
+ return TheCache.isPointerDereferencedInBlock(Val, BB, [DL](BasicBlock *BB) {
+ DenseSet<AssertingVH<Value>> DereferencedPointers;
for (Instruction &I : *BB)
- if (InstructionDereferencesPointer(&I, UnderlyingVal))
- return true;
- return false;
+ AddPointersDereferencedByInstruction(&I, DereferencedPointers, DL);
+ return DereferencedPointers;
+ });
}
bool LazyValueInfoImpl::solveBlockValueNonLocal(ValueLatticeElement &BBLV,
- Value *Val, BasicBlock *BB) {
+ Value *Val, BasicBlock *BB) {
ValueLatticeElement Result; // Start Undefined.
// If this is the entry block, we must be asking about an argument. The
// value is overdefined.
if (BB == &BB->getParent()->getEntryBlock()) {
assert(isa<Argument>(Val) && "Unknown live-in to the entry block");
- // Before giving up, see if we can prove the pointer non-null local to
- // this particular block.
- PointerType *PTy = dyn_cast<PointerType>(Val->getType());
- if (PTy &&
- (isKnownNonZero(Val, DL) ||
- (isObjectDereferencedInBlock(Val, BB) &&
- !NullPointerIsDefined(BB->getParent(), PTy->getAddressSpace())))) {
- Result = ValueLatticeElement::getNot(ConstantPointerNull::get(PTy));
- } else {
- Result = ValueLatticeElement::getOverdefined();
- }
- BBLV = Result;
+ BBLV = ValueLatticeElement::getOverdefined();
return true;
}
@@ -713,14 +722,6 @@ bool LazyValueInfoImpl::solveBlockValueNonLocal(ValueLatticeElement &BBLV,
if (Result.isOverdefined()) {
LLVM_DEBUG(dbgs() << " compute BB '" << BB->getName()
<< "' - overdefined because of pred (non local).\n");
- // Before giving up, see if we can prove the pointer non-null local to
- // this particular block.
- PointerType *PTy = dyn_cast<PointerType>(Val->getType());
- if (PTy && isObjectDereferencedInBlock(Val, BB) &&
- !NullPointerIsDefined(BB->getParent(), PTy->getAddressSpace())) {
- Result = ValueLatticeElement::getNot(ConstantPointerNull::get(PTy));
- }
-
BBLV = Result;
return true;
}
@@ -793,16 +794,24 @@ void LazyValueInfoImpl::intersectAssumeOrGuardBlockValueConstantRange(
// If guards are not used in the module, don't spend time looking for them
auto *GuardDecl = BBI->getModule()->getFunction(
Intrinsic::getName(Intrinsic::experimental_guard));
- if (!GuardDecl || GuardDecl->use_empty())
- return;
+ if (GuardDecl && !GuardDecl->use_empty()) {
+ if (BBI->getIterator() == BBI->getParent()->begin())
+ return;
+ for (Instruction &I : make_range(std::next(BBI->getIterator().getReverse()),
+ BBI->getParent()->rend())) {
+ Value *Cond = nullptr;
+ if (match(&I, m_Intrinsic<Intrinsic::experimental_guard>(m_Value(Cond))))
+ BBLV = intersect(BBLV, getValueFromCondition(Val, Cond));
+ }
+ }
- if (BBI->getIterator() == BBI->getParent()->begin())
- return;
- for (Instruction &I : make_range(std::next(BBI->getIterator().getReverse()),
- BBI->getParent()->rend())) {
- Value *Cond = nullptr;
- if (match(&I, m_Intrinsic<Intrinsic::experimental_guard>(m_Value(Cond))))
- BBLV = intersect(BBLV, getValueFromCondition(Val, Cond));
+ if (BBLV.isOverdefined()) {
+ // Check whether we're checking at the terminator, and the pointer has
+ // been dereferenced in this block.
+ PointerType *PTy = dyn_cast<PointerType>(Val->getType());
+ if (PTy && BBI->getParent()->getTerminator() == BBI &&
+ isNonNullDueToDereferenceInBlock(Val, BBI->getParent()))
+ BBLV = ValueLatticeElement::getNot(ConstantPointerNull::get(PTy));
}
}
diff --git a/llvm/test/Transforms/JumpThreading/combine-metadata.ll b/llvm/test/Transforms/JumpThreading/combine-metadata.ll
index 6351236aebbc..d7c390eb4b0a 100644
--- a/llvm/test/Transforms/JumpThreading/combine-metadata.ll
+++ b/llvm/test/Transforms/JumpThreading/combine-metadata.ll
@@ -108,7 +108,7 @@ d2:
d3:
%y = load i32*, i32** %ptr
store i32 1, i32* %y
- %c2 = icmp eq i32* %y, null
+ %c2 = icmp eq i32* %y, @p
br i1 %c2, label %ret1, label %ret2
ret1:
@@ -118,5 +118,6 @@ ret2:
ret void
}
+ at p = external global i32
!0 = !{}
More information about the llvm-commits
mailing list