[llvm] r290786 - [ValueTracking] make dominator tree requirement explicit for isKnownNonNullFromDominatingCondition(); NFCI

Sanjay Patel via llvm-commits llvm-commits at lists.llvm.org
Sat Dec 31 10:30:29 PST 2016


I think it's not bad; there are only 5 calls to isKnownNonNullAt() in the
whole project. So we could just hoist those checks to the callers as needed
and assert in here instead. Let me know if I've understood the suggestion.

What I was hoping to do next is change the call to isKnownNonNull() that is
currently in isKnownNonZero():
https://github.com/llvm-mirror/llvm/blob/master/lib/
Analysis/ValueTracking.cpp#L1834

into a call to isKnownNonNullAt(), so we'd have to add guard checks in the
caller there too if we do change the position of those checks.

It's not clear to me why we have this line:
https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/ValueTracking.cpp#L3409
...for non-context-sensitive analysis only in isKnownNonNullAt(), but not
in isKnownNonNull(), so I may not be understanding how this all works.

On Sat, Dec 31, 2016 at 10:58 AM, Davide Italiano <davide at freebsd.org>
wrote:

> On Sat, Dec 31, 2016 at 9:37 AM, Sanjay Patel via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> > Author: spatel
> > Date: Sat Dec 31 11:37:01 2016
> > New Revision: 290786
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=290786&view=rev
> > Log:
> > [ValueTracking] make dominator tree requirement explicit for
> isKnownNonNullFromDominatingCondition(); NFCI
> >
> > I don't think this hole is currently exposed, but I crashed regression
> tests for
> > jump-threading and loop-vectorize after I added calls to
> isKnownNonNullAt() in
> > InstSimplify as part of trying to solve PR28430:
> > https://llvm.org/bugs/show_bug.cgi?id=28430
> >
> > That's because they call into value tracking with a context instruction,
> but no
> > other parts of the query structure filled in.
> >
> > For more background, see the discussion in:
> > https://reviews.llvm.org/D27855
> >
> > Modified:
> >     llvm/trunk/include/llvm/Analysis/ValueTracking.h
> >     llvm/trunk/lib/Analysis/ValueTracking.cpp
> >
> > Modified: llvm/trunk/include/llvm/Analysis/ValueTracking.h
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/
> llvm/Analysis/ValueTracking.h?rev=290786&r1=290785&r2=290786&view=diff
> > ============================================================
> ==================
> > --- llvm/trunk/include/llvm/Analysis/ValueTracking.h (original)
> > +++ llvm/trunk/include/llvm/Analysis/ValueTracking.h Sat Dec 31
> 11:37:01 2016
> > @@ -308,12 +308,12 @@ template <typename T> class ArrayRef;
> >    bool isKnownNonNull(const Value *V);
> >
> >    /// Return true if this pointer couldn't possibly be null. If the
> context
> > -  /// instruction is specified, perform context-sensitive analysis and
> return
> > -  /// true if the pointer couldn't possibly be null at the specified
> > -  /// instruction.
> > +  /// instruction and dominator tree are specified, perform
> context-sensitive
> > +  /// analysis and return true if the pointer couldn't possibly be null
> at the
> > +  /// specified instruction.
> >    bool isKnownNonNullAt(const Value *V,
> >                          const Instruction *CtxI = nullptr,
> > -                        const DominatorTree *DT  = nullptr);
> > +                        const DominatorTree *DT = nullptr);
> >
> >    /// Return true if it is valid to use the assumptions provided by an
> >    /// assume intrinsic, I, at the point in the control-flow identified
> by the
> >
> > Modified: llvm/trunk/lib/Analysis/ValueTracking.cpp
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Analysis/ValueTracking.cpp?rev=290786&r1=290785&r2=290786&view=diff
> > ============================================================
> ==================
> > --- llvm/trunk/lib/Analysis/ValueTracking.cpp (original)
> > +++ llvm/trunk/lib/Analysis/ValueTracking.cpp Sat Dec 31 11:37:01 2016
> > @@ -3368,6 +3368,8 @@ static bool isKnownNonNullFromDominating
> >                                                    const DominatorTree
> *DT) {
> >    assert(V->getType()->isPointerTy() && "V must be pointer type");
> >    assert(!isa<ConstantData>(V) && "Did not expect ConstantPointerNull");
> > +  assert(CtxI && "Context instruction required for analysis");
> > +  assert(DT && "Dominator tree required for analysis");
> >
> >    unsigned NumUsesExplored = 0;
> >    for (auto *U : V->users()) {
> > @@ -3410,7 +3412,10 @@ bool llvm::isKnownNonNullAt(const Value
> >    if (isKnownNonNull(V))
> >      return true;
> >
> > -  return CtxI ? ::isKnownNonNullFromDominatingCondition(V, CtxI, DT) :
> false;
> > +  if (!CtxI || !DT)
> > +    return false;
> > +
>
> How hard is to guarantee that we always have a valid/updated dominator
> when we get here (i.e. we don't need to bail out)?
>
> --
> Davide
>
> "There are no solved problems; there are only problems that are more
> or less solved" -- Henri Poincare
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161231/9b98850d/attachment.html>


More information about the llvm-commits mailing list