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

[Sanjay Patel via llvm-commits]

Ah, that would be a stronger assert. I don't know if we can make such a
strong guarantee. As I mentioned in the original message, the loop
vectorizer and jump threading at least would not pass the test currently. I
think we would need to audit any caller of llvm::SimplifyInstruction?

For reference, my proposed value tracking change is here:
https://reviews.llvm.org/D28204

On Tue, Jan 3, 2017 at 4:36 AM, Davide Italiano <davide at freebsd.org> wrote:

> On Sat, Dec 31, 2016 at 10:30 AM, Sanjay Patel <spatel at rotateright.com>
> wrote:
> > 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.
> >
>
> I mean something like this:
>
> ```
> bool runOnFunction(Function &F, DominatorTree *DT, ...) {
>   assert(DT && "Dominator Tree required");
>   [...]
> }
> ```
> (i.e. we always guarantee that the domtree/AA/* we have is valid in
> order to guarantee that optimization will fire and we never bail out).
> Not sure how hard would be. Thoughts?
>
>
> > 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
> >
> >
>
>
>
> --
> 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/20170103/0eabb4c3/attachment.html>