[llvm] 885a05f - Reapply [LVI] Normalize pointer behavior
Eric Christopher via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 12 15:51:40 PST 2019
I'm going to go ahead and temporarily revert this for now since we've
got a bit of a reproducer and a place for discussion for now. Probably
causing python to fail to compile isn't going to be ok even if it
turns out to be a problem in python in the near term so let's revert
and discuss if we need to.
-eric
On Tue, Nov 12, 2019 at 9:13 AM Benjamin Kramer via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
> The crash is in insertdict() actually. The optimizer removed half the
> function https://github.com/python/cpython/blob/v3.6.9/Objects/dictobject.c#L1112
>
>
>
> On Tue, Nov 12, 2019 at 5:08 PM Benjamin Kramer <benny.kra at gmail.com> wrote:
> >
> > 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?
> >
> > On Fri, Nov 8, 2019 at 8:15 PM Nikita Popov via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> >>
> >>
> >> Author: Nikita Popov
> >> Date: 2019-11-08T20:13:55+01:00
> >> New Revision: 885a05f48a5d320946c89590b73a764e5884fe4f
> >>
> >> URL: https://github.com/llvm/llvm-project/commit/885a05f48a5d320946c89590b73a764e5884fe4f
> >> DIFF: https://github.com/llvm/llvm-project/commit/885a05f48a5d320946c89590b73a764e5884fe4f.diff
> >>
> >> LOG: Reapply [LVI] Normalize pointer behavior
> >>
> >> Fix cache invalidation by not guarding the dereferenced pointer cache
> >> erasure by SeenBlocks. SeenBlocks is only populated when actually
> >> caching a value in the block, which doesn't necessarily have to happen
> >> just because dereferenced pointers were calculated.
> >>
> >> -----
> >>
> >> 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 09c2241c5ce5..471e22b666e0 100644
> >> --- a/llvm/lib/Analysis/LazyValueInfo.cpp
> >> +++ b/llvm/lib/Analysis/LazyValueInfo.cpp
> >> @@ -151,6 +151,11 @@ namespace {
> >> /// This is the cache kept by LazyValueInfo which
> >> /// maintains information about queries across the clients' queries.
> >> class LazyValueInfoCache {
> >> + public:
> >> + typedef DenseMap<PoisoningVH<BasicBlock>, SmallPtrSet<Value *, 4>>
> >> + PerBlockValueCacheTy;
> >> +
> >> + private:
> >> /// This is all of the cached block information for exactly one Value*.
> >> /// The entries are sorted by the BasicBlock* of the
> >> /// entries, allowing us to do a lookup with a binary search.
> >> @@ -162,10 +167,6 @@ namespace {
> >> SmallDenseMap<PoisoningVH<BasicBlock>, ValueLatticeElement, 4> BlockVals;
> >> };
> >>
> >> - /// This tracks, on a per-block basis, the set of values that are
> >> - /// over-defined at the end of that block.
> >> - typedef DenseMap<PoisoningVH<BasicBlock>, SmallPtrSet<Value *, 4>>
> >> - OverDefinedCacheTy;
> >> /// Keep track of all blocks that we have ever seen, so we
> >> /// don't spend time removing unused blocks from our caches.
> >> DenseSet<PoisoningVH<BasicBlock> > SeenBlocks;
> >> @@ -173,7 +174,12 @@ namespace {
> >> /// This is all of the cached information for all values,
> >> /// mapped from Value* to key information.
> >> DenseMap<Value *, std::unique_ptr<ValueCacheEntryTy>> ValueCache;
> >> - OverDefinedCacheTy OverDefinedCache;
> >> + /// This tracks, on a per-block basis, the set of values that are
> >> + /// over-defined at the end of that block.
> >> + PerBlockValueCacheTy OverDefinedCache;
> >> + /// This tracks, on a per-block basis, the set of pointers that are
> >> + /// dereferenced in the block (and thus non-null at the end of the block).
> >> + PerBlockValueCacheTy DereferencedPointerCache;
> >>
> >>
> >> public:
> >> @@ -229,11 +235,17 @@ namespace {
> >> return BBI->second;
> >> }
> >>
> >> + std::pair<PerBlockValueCacheTy::iterator, bool>
> >> + getOrInitDereferencedPointers(BasicBlock *BB) {
> >> + return DereferencedPointerCache.try_emplace(BB);
> >> + }
> >> +
> >> /// clear - Empty the cache.
> >> void clear() {
> >> SeenBlocks.clear();
> >> ValueCache.clear();
> >> OverDefinedCache.clear();
> >> + DereferencedPointerCache.clear();
> >> }
> >>
> >> /// Inform the cache that a given value has been deleted.
> >> @@ -252,17 +264,22 @@ namespace {
> >> };
> >> }
> >>
> >> -void LazyValueInfoCache::eraseValue(Value *V) {
> >> - for (auto I = OverDefinedCache.begin(), E = OverDefinedCache.end(); I != E;) {
> >> +static void eraseValueFromPerBlockValueCache(
> >> + Value *V, LazyValueInfoCache::PerBlockValueCacheTy &Cache) {
> >> + for (auto I = Cache.begin(), E = Cache.end(); I != E;) {
> >> // Copy and increment the iterator immediately so we can erase behind
> >> // ourselves.
> >> auto Iter = I++;
> >> SmallPtrSetImpl<Value *> &ValueSet = Iter->second;
> >> ValueSet.erase(V);
> >> if (ValueSet.empty())
> >> - OverDefinedCache.erase(Iter);
> >> + Cache.erase(Iter);
> >> }
> >> +}
> >>
> >> +void LazyValueInfoCache::eraseValue(Value *V) {
> >> + eraseValueFromPerBlockValueCache(V, OverDefinedCache);
> >> + eraseValueFromPerBlockValueCache(V, DereferencedPointerCache);
> >> ValueCache.erase(V);
> >> }
> >>
> >> @@ -273,15 +290,17 @@ void LVIValueHandle::deleted() {
> >> }
> >>
> >> void LazyValueInfoCache::eraseBlock(BasicBlock *BB) {
> >> + // The SeenBlocks shortcut applies only to the value caches,
> >> + // always clear the dereferenced pointer cache.
> >> + DereferencedPointerCache.erase(BB);
> >> +
> >> // Shortcut if we have never seen this block.
> >> DenseSet<PoisoningVH<BasicBlock> >::iterator I = SeenBlocks.find(BB);
> >> if (I == SeenBlocks.end())
> >> return;
> >> SeenBlocks.erase(I);
> >>
> >> - auto ODI = OverDefinedCache.find(BB);
> >> - if (ODI != OverDefinedCache.end())
> >> - OverDefinedCache.erase(ODI);
> >> + OverDefinedCache.erase(BB);
> >>
> >> for (auto &I : ValueCache)
> >> I.second->BlockVals.erase(BB);
> >> @@ -438,6 +457,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);
> >> @@ -619,17 +639,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,
> >> @@ -639,11 +648,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);
> >> @@ -664,75 +684,63 @@ 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;
> >> +static void AddDereferencedPointer(
> >> + Value *Ptr, SmallPtrSet<Value *, 4> &PtrSet, const DataLayout &DL) {
> >> + // TODO: Use NullPointerIsDefined instead.
> >> + if (Ptr->getType()->getPointerAddressSpace() == 0) {
> >> + Ptr = GetUnderlyingObject(Ptr, DL);
> >> + PtrSet.insert(Ptr);
> >> }
> >> - if (StoreInst *S = dyn_cast<StoreInst>(I)) {
> >> - return S->getPointerAddressSpace() == 0 &&
> >> - GetUnderlyingObject(S->getPointerOperand(),
> >> - S->getModule()->getDataLayout()) == Ptr;
> >> - }
> >> - if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(I)) {
> >> - if (MI->isVolatile()) return false;
> >> +}
> >> +
> >> +static void AddPointersDereferencedByInstruction(
> >> + Instruction *I, SmallPtrSet<Value *, 4> &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);
> >> +
> >> + LazyValueInfoCache::PerBlockValueCacheTy::iterator It;
> >> + bool NeedsInit;
> >> + std::tie(It, NeedsInit) = TheCache.getOrInitDereferencedPointers(BB);
> >> +
> >> + if (NeedsInit)
> >> for (Instruction &I : *BB)
> >> - if (InstructionDereferencesPointer(&I, UnderlyingVal))
> >> - return true;
> >> - return false;
> >> + AddPointersDereferencedByInstruction(&I, It->second, DL);
> >> +
> >> + return It->second.count(Val);
> >> }
> >>
> >> 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;
> >> }
> >>
> >> @@ -758,14 +766,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;
> >> }
> >> @@ -838,16 +838,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 = !{}
> >>
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list