[llvm] 885a05f - Reapply [LVI] Normalize pointer behavior
Eric Christopher via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 12 15:53:19 PST 2019
Reverted here for now:
echristo at jhereg ~/s/llvm-project> git push
To github.com:llvm/llvm-project.git
056c3197694..7a3ad48d6de master -> master
Sorry for any issues.
-eric
On Tue, Nov 12, 2019 at 3:51 PM Eric Christopher <echristo at gmail.com> wrote:
>
> 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