[llvm] 6d88f6e - Reapply [LVI] Normalize pointer behavior
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Sat Aug 29 12:17:14 PDT 2020
Author: Nikita Popov
Date: 2020-08-29T21:17:03+02:00
New Revision: 6d88f6efd44852b995de3a1620ea67b6381c1ad9
URL: https://github.com/llvm/llvm-project/commit/6d88f6efd44852b995de3a1620ea67b6381c1ad9
DIFF: https://github.com/llvm/llvm-project/commit/6d88f6efd44852b995de3a1620ea67b6381c1ad9.diff
LOG: Reapply [LVI] Normalize pointer behavior
This got reverted because a dependency was reverted. It has since
been reapplied, so reapply this as well.
-----
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 more powerful than the previous implementation as
a side-effect, and this does actually seem benefitial in practice.
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/CorrelatedValuePropagation/non-null.ll
Removed:
################################################################################
diff --git a/llvm/lib/Analysis/LazyValueInfo.cpp b/llvm/lib/Analysis/LazyValueInfo.cpp
index 55c9808f541e..8a0507ffc268 100644
--- a/llvm/lib/Analysis/LazyValueInfo.cpp
+++ b/llvm/lib/Analysis/LazyValueInfo.cpp
@@ -150,15 +150,21 @@ namespace {
} // end anonymous namespace
namespace {
+ using NonNullPointerSet = SmallDenseSet<AssertingVH<Value>, 2>;
+
/// This is the cache kept by LazyValueInfo which
/// maintains information about queries across the clients' queries.
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 BlockCacheEntry {
SmallDenseMap<AssertingVH<Value>, ValueLatticeElement, 4> LatticeElements;
SmallDenseSet<AssertingVH<Value>, 4> OverDefined;
+ // None indicates that the nonnull pointers for this basic block
+ // block have not been computed yet.
+ Optional<NonNullPointerSet> NonNullPointers;
};
/// Cached information per basic block.
@@ -220,6 +226,19 @@ namespace {
return LatticeIt->second;
}
+ bool isNonNullAtEndOfBlock(
+ Value *V, BasicBlock *BB,
+ function_ref<NonNullPointerSet(BasicBlock *)> InitFn) {
+ BlockCacheEntry *Entry = getOrCreateBlockEntry(BB);
+ if (!Entry->NonNullPointers) {
+ Entry->NonNullPointers = InitFn(BB);
+ for (Value *V : *Entry->NonNullPointers)
+ addValueHandle(V);
+ }
+
+ return Entry->NonNullPointers->count(V);
+ }
+
/// clear - Empty the cache.
void clear() {
BlockCache.clear();
@@ -244,6 +263,8 @@ void LazyValueInfoCache::eraseValue(Value *V) {
for (auto &Pair : BlockCache) {
Pair.second->LatticeElements.erase(V);
Pair.second->OverDefined.erase(V);
+ if (Pair.second->NonNullPointers)
+ Pair.second->NonNullPointers->erase(V);
}
auto HandleIt = ValueHandles.find_as(V);
@@ -404,6 +425,7 @@ class LazyValueInfoImpl {
BasicBlock *BB);
Optional<ValueLatticeElement> solveBlockValueExtractValue(
ExtractValueInst *EVI, BasicBlock *BB);
+ bool isNonNullAtEndOfBlock(Value *Val, BasicBlock *BB);
void intersectAssumeOrGuardBlockValueConstantRange(Value *Val,
ValueLatticeElement &BBLV,
Instruction *BBI);
@@ -603,48 +625,43 @@ Optional<ValueLatticeElement> LazyValueInfoImpl::solveBlockValueImpl(
return getFromRangeMetadata(BBI);
}
-static bool InstructionDereferencesPointer(Instruction *I, Value *Ptr) {
+static void AddNonNullPointer(Value *Ptr, NonNullPointerSet &PtrSet) {
+ // TODO: Use NullPointerIsDefined instead.
+ if (Ptr->getType()->getPointerAddressSpace() == 0)
+ PtrSet.insert(getUnderlyingObject(Ptr));
+}
+
+static void AddNonNullPointersByInstruction(
+ Instruction *I, NonNullPointerSet &PtrSet) {
if (LoadInst *L = dyn_cast<LoadInst>(I)) {
- return L->getPointerAddressSpace() == 0 &&
- getUnderlyingObject(L->getPointerOperand()) == Ptr;
- }
- if (StoreInst *S = dyn_cast<StoreInst>(I)) {
- return S->getPointerAddressSpace() == 0 &&
- getUnderlyingObject(S->getPointerOperand()) == Ptr;
- }
- if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(I)) {
- if (MI->isVolatile()) return false;
+ AddNonNullPointer(L->getPointerOperand(), PtrSet);
+ } else if (StoreInst *S = dyn_cast<StoreInst>(I)) {
+ AddNonNullPointer(S->getPointerOperand(), PtrSet);
+ } 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()) == Ptr)
- return true;
+ AddNonNullPointer(MI->getRawDest(), PtrSet);
if (MemTransferInst *MTI = dyn_cast<MemTransferInst>(MI))
- if (MTI->getSourceAddressSpace() == 0)
- if (getUnderlyingObject(MTI->getRawSource()) == Ptr)
- return true;
+ AddNonNullPointer(MTI->getRawSource(), PtrSet);
}
- 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::isNonNullAtEndOfBlock(Value *Val, BasicBlock *BB) {
+ if (NullPointerIsDefined(BB->getParent(),
+ Val->getType()->getPointerAddressSpace()))
+ return false;
- Value *UnderlyingVal = getUnderlyingObject(Val);
- // If 'getUnderlyingObject' didn't converge, skip it. It won't converge
- // inside InstructionDereferencesPointer either.
- if (UnderlyingVal == getUnderlyingObject(UnderlyingVal, 1))
+ Val = getUnderlyingObject(Val);
+ return TheCache.isNonNullAtEndOfBlock(Val, BB, [this](BasicBlock *BB) {
+ NonNullPointerSet NonNullPointers;
for (Instruction &I : *BB)
- if (InstructionDereferencesPointer(&I, UnderlyingVal))
- return true;
- return false;
+ AddNonNullPointersByInstruction(&I, NonNullPointers);
+ return NonNullPointers;
+ });
}
Optional<ValueLatticeElement> LazyValueInfoImpl::solveBlockValueNonLocal(
@@ -655,16 +672,7 @@ Optional<ValueLatticeElement> LazyValueInfoImpl::solveBlockValueNonLocal(
// 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()))))
- return ValueLatticeElement::getNot(ConstantPointerNull::get(PTy));
- else
- return ValueLatticeElement::getOverdefined();
+ return ValueLatticeElement::getOverdefined();
}
// Loop over all of our predecessors, merging what we know from them into
@@ -689,14 +697,6 @@ Optional<ValueLatticeElement> LazyValueInfoImpl::solveBlockValueNonLocal(
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));
- }
-
return Result;
}
}
@@ -769,16 +769,23 @@ void LazyValueInfoImpl::intersectAssumeOrGuardBlockValueConstantRange(
}
// If guards are not used in the module, don't spend time looking for them
- if (!GuardDecl || GuardDecl->use_empty())
- return;
+ if (GuardDecl && !GuardDecl->use_empty() &&
+ BBI->getIterator() != BB->begin()) {
+ for (Instruction &I : make_range(std::next(BBI->getIterator().getReverse()),
+ BB->rend())) {
+ Value *Cond = nullptr;
+ if (match(&I, m_Intrinsic<Intrinsic::experimental_guard>(m_Value(Cond))))
+ BBLV = intersect(BBLV, getValueFromCondition(Val, Cond));
+ }
+ }
- if (BBI->getIterator() == BB->begin())
- return;
- for (Instruction &I : make_range(std::next(BBI->getIterator().getReverse()),
- BB->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 && BB->getTerminator() == BBI &&
+ isNonNullAtEndOfBlock(Val, BB))
+ BBLV = ValueLatticeElement::getNot(ConstantPointerNull::get(PTy));
}
}
diff --git a/llvm/test/Transforms/CorrelatedValuePropagation/non-null.ll b/llvm/test/Transforms/CorrelatedValuePropagation/non-null.ll
index e2f6b798b970..672bd170a932 100644
--- a/llvm/test/Transforms/CorrelatedValuePropagation/non-null.ll
+++ b/llvm/test/Transforms/CorrelatedValuePropagation/non-null.ll
@@ -337,7 +337,7 @@ define i1 @test_store_same_block(i8* %arg) {
; CHECK-LABEL: @test_store_same_block(
; CHECK-NEXT: store i8 0, i8* [[ARG:%.*]], align 1
; CHECK-NEXT: [[CMP:%.*]] = icmp ne i8* [[ARG]], null
-; CHECK-NEXT: ret i1 [[CMP]]
+; CHECK-NEXT: ret i1 true
;
store i8 0, i8* %arg
%cmp = icmp ne i8* %arg, null
More information about the llvm-commits
mailing list