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

[Davide Italiano via llvm-commits]

On Tue, Jan 3, 2017 at 7:50 AM, Sanjay Patel <spatel at rotateright.com> wrote:
> 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?
>

Well, at least we could start understanding why the Dominator is not
available in those passes (and see if it can be fixed instead of
bailing out).

> 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
>
>



-- 
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare