<div dir="ltr">I'm seeing a miscompile of Python 3.6's dictobject.c after this change. Attached are before/after IR dumps. It manifests as a null dereference in _PyObjectDict_SetItem. Do you see anything suspicious?<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Nov 8, 2019 at 8:15 PM Nikita Popov via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Author: Nikita Popov<br>
Date: 2019-11-08T20:13:55+01:00<br>
New Revision: 885a05f48a5d320946c89590b73a764e5884fe4f<br>
<br>
URL: <a href="https://github.com/llvm/llvm-project/commit/885a05f48a5d320946c89590b73a764e5884fe4f" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/885a05f48a5d320946c89590b73a764e5884fe4f</a><br>
DIFF: <a href="https://github.com/llvm/llvm-project/commit/885a05f48a5d320946c89590b73a764e5884fe4f.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/885a05f48a5d320946c89590b73a764e5884fe4f.diff</a><br>
<br>
LOG: Reapply [LVI] Normalize pointer behavior<br>
<br>
Fix cache invalidation by not guarding the dereferenced pointer cache<br>
erasure by SeenBlocks. SeenBlocks is only populated when actually<br>
caching a value in the block, which doesn't necessarily have to happen<br>
just because dereferenced pointers were calculated.<br>
<br>
-----<br>
<br>
Related to D69686. As noted there, LVI currently behaves differently<br>
for integer and pointer values: For integers, the block value is always<br>
valid inside the basic block, while for pointers it is only valid at<br>
the end of the basic block. I believe the integer behavior is the<br>
correct one, and CVP relies on it via its getConstantRange() uses.<br>
<br>
The reason for the special pointer behavior is that LVI checks whether<br>
a pointer is dereferenced in a given basic block and marks it as<br>
non-null in that case. Of course, this information is valid only after<br>
the dereferencing instruction, or in conservative approximation,<br>
at the end of the block.<br>
<br>
This patch changes the treatment of dereferencability: Instead of<br>
including it inside the block value, we instead treat it as something<br>
similar to an assume (it essentially is a non-nullness assume) and<br>
incorporate this information in intersectAssumeOrGuardBlockValueConstantRange()<br>
if the context instruction is the terminator of the basic block.<br>
This happens either when determining an edge-value internally in LVI,<br>
or when a terminator was explicitly passed to getValueAt(). The latter<br>
case makes this change not fully NFC, because we can now fold<br>
terminator icmps based on the dereferencability information in the<br>
same block. This is the reason why I changed one JumpThreading test<br>
(it would optimize the condition away without the change).<br>
<br>
Of course, we do not want to recompute dereferencability on each<br>
intersectAssume call, so we need a new cache for this. The<br>
dereferencability analysis requires walking the entire basic block<br>
and computing underlying objects of all memory operands. This was<br>
previously done separately for each queried pointer value. In the<br>
new implementation (both because this makes the caching simpler,<br>
and because it is faster), I instead only walk the full BB once and<br>
cache all the dereferenced pointers. So the traversal is now performed<br>
only once per BB, instead of once per queried pointer value.<br>
<br>
I think the overall model now makes more sense than before, and there<br>
will be no more pitfalls due to differing integer/pointer behavior.<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D69914" rel="noreferrer" target="_blank">https://reviews.llvm.org/D69914</a><br>
<br>
Added: <br>
<br>
<br>
Modified: <br>
llvm/lib/Analysis/LazyValueInfo.cpp<br>
llvm/test/Transforms/JumpThreading/combine-metadata.ll<br>
<br>
Removed: <br>
<br>
<br>
<br>
################################################################################<br>
diff --git a/llvm/lib/Analysis/LazyValueInfo.cpp b/llvm/lib/Analysis/LazyValueInfo.cpp<br>
index 09c2241c5ce5..471e22b666e0 100644<br>
--- a/llvm/lib/Analysis/LazyValueInfo.cpp<br>
+++ b/llvm/lib/Analysis/LazyValueInfo.cpp<br>
@@ -151,6 +151,11 @@ namespace {<br>
/// This is the cache kept by LazyValueInfo which<br>
/// maintains information about queries across the clients' queries.<br>
class LazyValueInfoCache {<br>
+ public:<br>
+ typedef DenseMap<PoisoningVH<BasicBlock>, SmallPtrSet<Value *, 4>><br>
+ PerBlockValueCacheTy;<br>
+<br>
+ private:<br>
/// This is all of the cached block information for exactly one Value*.<br>
/// The entries are sorted by the BasicBlock* of the<br>
/// entries, allowing us to do a lookup with a binary search.<br>
@@ -162,10 +167,6 @@ namespace {<br>
SmallDenseMap<PoisoningVH<BasicBlock>, ValueLatticeElement, 4> BlockVals;<br>
};<br>
<br>
- /// This tracks, on a per-block basis, the set of values that are<br>
- /// over-defined at the end of that block.<br>
- typedef DenseMap<PoisoningVH<BasicBlock>, SmallPtrSet<Value *, 4>><br>
- OverDefinedCacheTy;<br>
/// Keep track of all blocks that we have ever seen, so we<br>
/// don't spend time removing unused blocks from our caches.<br>
DenseSet<PoisoningVH<BasicBlock> > SeenBlocks;<br>
@@ -173,7 +174,12 @@ namespace {<br>
/// This is all of the cached information for all values,<br>
/// mapped from Value* to key information.<br>
DenseMap<Value *, std::unique_ptr<ValueCacheEntryTy>> ValueCache;<br>
- OverDefinedCacheTy OverDefinedCache;<br>
+ /// This tracks, on a per-block basis, the set of values that are<br>
+ /// over-defined at the end of that block.<br>
+ PerBlockValueCacheTy OverDefinedCache;<br>
+ /// This tracks, on a per-block basis, the set of pointers that are<br>
+ /// dereferenced in the block (and thus non-null at the end of the block).<br>
+ PerBlockValueCacheTy DereferencedPointerCache;<br>
<br>
<br>
public:<br>
@@ -229,11 +235,17 @@ namespace {<br>
return BBI->second;<br>
}<br>
<br>
+ std::pair<PerBlockValueCacheTy::iterator, bool><br>
+ getOrInitDereferencedPointers(BasicBlock *BB) {<br>
+ return DereferencedPointerCache.try_emplace(BB);<br>
+ }<br>
+<br>
/// clear - Empty the cache.<br>
void clear() {<br>
SeenBlocks.clear();<br>
ValueCache.clear();<br>
OverDefinedCache.clear();<br>
+ DereferencedPointerCache.clear();<br>
}<br>
<br>
/// Inform the cache that a given value has been deleted.<br>
@@ -252,17 +264,22 @@ namespace {<br>
};<br>
}<br>
<br>
-void LazyValueInfoCache::eraseValue(Value *V) {<br>
- for (auto I = OverDefinedCache.begin(), E = OverDefinedCache.end(); I != E;) {<br>
+static void eraseValueFromPerBlockValueCache(<br>
+ Value *V, LazyValueInfoCache::PerBlockValueCacheTy &Cache) {<br>
+ for (auto I = Cache.begin(), E = Cache.end(); I != E;) {<br>
// Copy and increment the iterator immediately so we can erase behind<br>
// ourselves.<br>
auto Iter = I++;<br>
SmallPtrSetImpl<Value *> &ValueSet = Iter->second;<br>
ValueSet.erase(V);<br>
if (ValueSet.empty())<br>
- OverDefinedCache.erase(Iter);<br>
+ Cache.erase(Iter);<br>
}<br>
+}<br>
<br>
+void LazyValueInfoCache::eraseValue(Value *V) {<br>
+ eraseValueFromPerBlockValueCache(V, OverDefinedCache);<br>
+ eraseValueFromPerBlockValueCache(V, DereferencedPointerCache);<br>
ValueCache.erase(V);<br>
}<br>
<br>
@@ -273,15 +290,17 @@ void LVIValueHandle::deleted() {<br>
}<br>
<br>
void LazyValueInfoCache::eraseBlock(BasicBlock *BB) {<br>
+ // The SeenBlocks shortcut applies only to the value caches,<br>
+ // always clear the dereferenced pointer cache.<br>
+ DereferencedPointerCache.erase(BB);<br>
+<br>
// Shortcut if we have never seen this block.<br>
DenseSet<PoisoningVH<BasicBlock> >::iterator I = SeenBlocks.find(BB);<br>
if (I == SeenBlocks.end())<br>
return;<br>
SeenBlocks.erase(I);<br>
<br>
- auto ODI = OverDefinedCache.find(BB);<br>
- if (ODI != OverDefinedCache.end())<br>
- OverDefinedCache.erase(ODI);<br>
+ OverDefinedCache.erase(BB);<br>
<br>
for (auto &I : ValueCache)<br>
I.second->BlockVals.erase(BB);<br>
@@ -438,6 +457,7 @@ namespace {<br>
BasicBlock *BB);<br>
bool solveBlockValueExtractValue(ValueLatticeElement &BBLV,<br>
ExtractValueInst *EVI, BasicBlock *BB);<br>
+ bool isNonNullDueToDereferenceInBlock(Value *Val, BasicBlock *BB);<br>
void intersectAssumeOrGuardBlockValueConstantRange(Value *Val,<br>
ValueLatticeElement &BBLV,<br>
Instruction *BBI);<br>
@@ -619,17 +639,6 @@ bool LazyValueInfoImpl::solveBlockValue(Value *Val, BasicBlock *BB) {<br>
<br>
bool LazyValueInfoImpl::solveBlockValueImpl(ValueLatticeElement &Res,<br>
Value *Val, BasicBlock *BB) {<br>
-<br>
- Instruction *BBI = dyn_cast<Instruction>(Val);<br>
- if (!BBI || BBI->getParent() != BB)<br>
- return solveBlockValueNonLocal(Res, Val, BB);<br>
-<br>
- if (PHINode *PN = dyn_cast<PHINode>(BBI))<br>
- return solveBlockValuePHINode(Res, PN, BB);<br>
-<br>
- if (auto *SI = dyn_cast<SelectInst>(BBI))<br>
- return solveBlockValueSelect(Res, SI, BB);<br>
-<br>
// If this value is a nonnull pointer, record it's range and bailout. Note<br>
// that for all other pointer typed values, we terminate the search at the<br>
// definition. We could easily extend this to look through geps, bitcasts,<br>
@@ -639,11 +648,22 @@ bool LazyValueInfoImpl::solveBlockValueImpl(ValueLatticeElement &Res,<br>
// This does mean that we have a sensitivity to where the defining<br>
// instruction is placed, even if it could legally be hoisted much higher.<br>
// That is unfortunate.<br>
- PointerType *PT = dyn_cast<PointerType>(BBI->getType());<br>
- if (PT && isKnownNonZero(BBI, DL)) {<br>
+ PointerType *PT = dyn_cast<PointerType>(Val->getType());<br>
+ if (PT && isKnownNonZero(Val, DL)) {<br>
Res = ValueLatticeElement::getNot(ConstantPointerNull::get(PT));<br>
return true;<br>
}<br>
+<br>
+ Instruction *BBI = dyn_cast<Instruction>(Val);<br>
+ if (!BBI || BBI->getParent() != BB)<br>
+ return solveBlockValueNonLocal(Res, Val, BB);<br>
+<br>
+ if (PHINode *PN = dyn_cast<PHINode>(BBI))<br>
+ return solveBlockValuePHINode(Res, PN, BB);<br>
+<br>
+ if (auto *SI = dyn_cast<SelectInst>(BBI))<br>
+ return solveBlockValueSelect(Res, SI, BB);<br>
+<br>
if (BBI->getType()->isIntegerTy()) {<br>
if (auto *CI = dyn_cast<CastInst>(BBI))<br>
return solveBlockValueCast(Res, CI, BB);<br>
@@ -664,75 +684,63 @@ bool LazyValueInfoImpl::solveBlockValueImpl(ValueLatticeElement &Res,<br>
return true;<br>
}<br>
<br>
-static bool InstructionDereferencesPointer(Instruction *I, Value *Ptr) {<br>
- if (LoadInst *L = dyn_cast<LoadInst>(I)) {<br>
- return L->getPointerAddressSpace() == 0 &&<br>
- GetUnderlyingObject(L->getPointerOperand(),<br>
- L->getModule()->getDataLayout()) == Ptr;<br>
+static void AddDereferencedPointer(<br>
+ Value *Ptr, SmallPtrSet<Value *, 4> &PtrSet, const DataLayout &DL) {<br>
+ // TODO: Use NullPointerIsDefined instead.<br>
+ if (Ptr->getType()->getPointerAddressSpace() == 0) {<br>
+ Ptr = GetUnderlyingObject(Ptr, DL);<br>
+ PtrSet.insert(Ptr);<br>
}<br>
- if (StoreInst *S = dyn_cast<StoreInst>(I)) {<br>
- return S->getPointerAddressSpace() == 0 &&<br>
- GetUnderlyingObject(S->getPointerOperand(),<br>
- S->getModule()->getDataLayout()) == Ptr;<br>
- }<br>
- if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(I)) {<br>
- if (MI->isVolatile()) return false;<br>
+}<br>
+<br>
+static void AddPointersDereferencedByInstruction(<br>
+ Instruction *I, SmallPtrSet<Value *, 4> &PtrSet, const DataLayout &DL) {<br>
+ if (LoadInst *L = dyn_cast<LoadInst>(I)) {<br>
+ AddDereferencedPointer(L->getPointerOperand(), PtrSet, DL);<br>
+ } else if (StoreInst *S = dyn_cast<StoreInst>(I)) {<br>
+ AddDereferencedPointer(S->getPointerOperand(), PtrSet, DL);<br>
+ } else if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(I)) {<br>
+ if (MI->isVolatile()) return;<br>
<br>
// FIXME: check whether it has a valuerange that excludes zero?<br>
ConstantInt *Len = dyn_cast<ConstantInt>(MI->getLength());<br>
- if (!Len || Len->isZero()) return false;<br>
+ if (!Len || Len->isZero()) return;<br>
<br>
- if (MI->getDestAddressSpace() == 0)<br>
- if (GetUnderlyingObject(MI->getRawDest(),<br>
- MI->getModule()->getDataLayout()) == Ptr)<br>
- return true;<br>
+ AddDereferencedPointer(MI->getRawDest(), PtrSet, DL);<br>
if (MemTransferInst *MTI = dyn_cast<MemTransferInst>(MI))<br>
- if (MTI->getSourceAddressSpace() == 0)<br>
- if (GetUnderlyingObject(MTI->getRawSource(),<br>
- MTI->getModule()->getDataLayout()) == Ptr)<br>
- return true;<br>
+ AddDereferencedPointer(MTI->getRawSource(), PtrSet, DL);<br>
}<br>
- return false;<br>
}<br>
<br>
-/// Return true if the allocation associated with Val is ever dereferenced<br>
-/// within the given basic block. This establishes the fact Val is not null,<br>
-/// but does not imply that the memory at Val is dereferenceable. (Val may<br>
-/// point off the end of the dereferenceable part of the object.)<br>
-static bool isObjectDereferencedInBlock(Value *Val, BasicBlock *BB) {<br>
- assert(Val->getType()->isPointerTy());<br>
+bool LazyValueInfoImpl::isNonNullDueToDereferenceInBlock(<br>
+ Value *Val, BasicBlock *BB) {<br>
+ if (NullPointerIsDefined(BB->getParent(),<br>
+ Val->getType()->getPointerAddressSpace()))<br>
+ return false;<br>
<br>
const DataLayout &DL = BB->getModule()->getDataLayout();<br>
- Value *UnderlyingVal = GetUnderlyingObject(Val, DL);<br>
- // If 'GetUnderlyingObject' didn't converge, skip it. It won't converge<br>
- // inside InstructionDereferencesPointer either.<br>
- if (UnderlyingVal == GetUnderlyingObject(UnderlyingVal, DL, 1))<br>
+ Val = GetUnderlyingObject(Val, DL);<br>
+<br>
+ LazyValueInfoCache::PerBlockValueCacheTy::iterator It;<br>
+ bool NeedsInit;<br>
+ std::tie(It, NeedsInit) = TheCache.getOrInitDereferencedPointers(BB);<br>
+<br>
+ if (NeedsInit)<br>
for (Instruction &I : *BB)<br>
- if (InstructionDereferencesPointer(&I, UnderlyingVal))<br>
- return true;<br>
- return false;<br>
+ AddPointersDereferencedByInstruction(&I, It->second, DL);<br>
+<br>
+ return It->second.count(Val);<br>
}<br>
<br>
bool LazyValueInfoImpl::solveBlockValueNonLocal(ValueLatticeElement &BBLV,<br>
- Value *Val, BasicBlock *BB) {<br>
+ Value *Val, BasicBlock *BB) {<br>
ValueLatticeElement Result; // Start Undefined.<br>
<br>
// If this is the entry block, we must be asking about an argument. The<br>
// value is overdefined.<br>
if (BB == &BB->getParent()->getEntryBlock()) {<br>
assert(isa<Argument>(Val) && "Unknown live-in to the entry block");<br>
- // Before giving up, see if we can prove the pointer non-null local to<br>
- // this particular block.<br>
- PointerType *PTy = dyn_cast<PointerType>(Val->getType());<br>
- if (PTy &&<br>
- (isKnownNonZero(Val, DL) ||<br>
- (isObjectDereferencedInBlock(Val, BB) &&<br>
- !NullPointerIsDefined(BB->getParent(), PTy->getAddressSpace())))) {<br>
- Result = ValueLatticeElement::getNot(ConstantPointerNull::get(PTy));<br>
- } else {<br>
- Result = ValueLatticeElement::getOverdefined();<br>
- }<br>
- BBLV = Result;<br>
+ BBLV = ValueLatticeElement::getOverdefined();<br>
return true;<br>
}<br>
<br>
@@ -758,14 +766,6 @@ bool LazyValueInfoImpl::solveBlockValueNonLocal(ValueLatticeElement &BBLV,<br>
if (Result.isOverdefined()) {<br>
LLVM_DEBUG(dbgs() << " compute BB '" << BB->getName()<br>
<< "' - overdefined because of pred (non local).\n");<br>
- // Before giving up, see if we can prove the pointer non-null local to<br>
- // this particular block.<br>
- PointerType *PTy = dyn_cast<PointerType>(Val->getType());<br>
- if (PTy && isObjectDereferencedInBlock(Val, BB) &&<br>
- !NullPointerIsDefined(BB->getParent(), PTy->getAddressSpace())) {<br>
- Result = ValueLatticeElement::getNot(ConstantPointerNull::get(PTy));<br>
- }<br>
-<br>
BBLV = Result;<br>
return true;<br>
}<br>
@@ -838,16 +838,24 @@ void LazyValueInfoImpl::intersectAssumeOrGuardBlockValueConstantRange(<br>
// If guards are not used in the module, don't spend time looking for them<br>
auto *GuardDecl = BBI->getModule()->getFunction(<br>
Intrinsic::getName(Intrinsic::experimental_guard));<br>
- if (!GuardDecl || GuardDecl->use_empty())<br>
- return;<br>
+ if (GuardDecl && !GuardDecl->use_empty()) {<br>
+ if (BBI->getIterator() == BBI->getParent()->begin())<br>
+ return;<br>
+ for (Instruction &I : make_range(std::next(BBI->getIterator().getReverse()),<br>
+ BBI->getParent()->rend())) {<br>
+ Value *Cond = nullptr;<br>
+ if (match(&I, m_Intrinsic<Intrinsic::experimental_guard>(m_Value(Cond))))<br>
+ BBLV = intersect(BBLV, getValueFromCondition(Val, Cond));<br>
+ }<br>
+ }<br>
<br>
- if (BBI->getIterator() == BBI->getParent()->begin())<br>
- return;<br>
- for (Instruction &I : make_range(std::next(BBI->getIterator().getReverse()),<br>
- BBI->getParent()->rend())) {<br>
- Value *Cond = nullptr;<br>
- if (match(&I, m_Intrinsic<Intrinsic::experimental_guard>(m_Value(Cond))))<br>
- BBLV = intersect(BBLV, getValueFromCondition(Val, Cond));<br>
+ if (BBLV.isOverdefined()) {<br>
+ // Check whether we're checking at the terminator, and the pointer has<br>
+ // been dereferenced in this block.<br>
+ PointerType *PTy = dyn_cast<PointerType>(Val->getType());<br>
+ if (PTy && BBI->getParent()->getTerminator() == BBI &&<br>
+ isNonNullDueToDereferenceInBlock(Val, BBI->getParent()))<br>
+ BBLV = ValueLatticeElement::getNot(ConstantPointerNull::get(PTy));<br>
}<br>
}<br>
<br>
<br>
diff --git a/llvm/test/Transforms/JumpThreading/combine-metadata.ll b/llvm/test/Transforms/JumpThreading/combine-metadata.ll<br>
index 6351236aebbc..d7c390eb4b0a 100644<br>
--- a/llvm/test/Transforms/JumpThreading/combine-metadata.ll<br>
+++ b/llvm/test/Transforms/JumpThreading/combine-metadata.ll<br>
@@ -108,7 +108,7 @@ d2:<br>
d3:<br>
%y = load i32*, i32** %ptr<br>
store i32 1, i32* %y<br>
- %c2 = icmp eq i32* %y, null<br>
+ %c2 = icmp eq i32* %y, @p<br>
br i1 %c2, label %ret1, label %ret2<br>
<br>
ret1:<br>
@@ -118,5 +118,6 @@ ret2:<br>
ret void<br>
}<br>
<br>
+@p = external global i32<br>
<br>
!0 = !{}<br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>