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

[Davide Italiano via llvm-commits]

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