<div dir="ltr"><div><div>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.<br><br></div>What I was hoping to do next is change the call to isKnownNonNull() that is currently in isKnownNonZero():<br><a href="https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/ValueTracking.cpp#L1834" target="_blank">https://github.com/llvm-<wbr>mirror/llvm/blob/master/lib/<wbr>Analysis/ValueTracking.cpp#<wbr>L1834</a><br><br></div>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. <br><br>It's not clear to me why we have this line:<br><div><div><a href="https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/ValueTracking.cpp#L3409">https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/ValueTracking.cpp#L3409</a><br></div><div>...for non-context-sensitive analysis only in isKnownNonNullAt(), but not in isKnownNonNull(), so I may not be understanding how this all works.<br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Dec 31, 2016 at 10:58 AM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Sat, Dec 31, 2016 at 9:37 AM, Sanjay Patel via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: spatel<br>
> Date: Sat Dec 31 11:37:01 2016<br>
> New Revision: 290786<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=290786&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=290786&view=rev</a><br>
> Log:<br>
> [ValueTracking] make dominator tree requirement explicit for isKnownNonNullFromDominatingCo<wbr>ndition(); NFCI<br>
><br>
> I don't think this hole is currently exposed, but I crashed regression tests for<br>
> jump-threading and loop-vectorize after I added calls to isKnownNonNullAt() in<br>
> InstSimplify as part of trying to solve PR28430:<br>
> <a href="https://llvm.org/bugs/show_bug.cgi?id=28430" rel="noreferrer" target="_blank">https://llvm.org/bugs/show_<wbr>bug.cgi?id=28430</a><br>
><br>
> That's because they call into value tracking with a context instruction, but no<br>
> other parts of the query structure filled in.<br>
><br>
> For more background, see the discussion in:<br>
> <a href="https://reviews.llvm.org/D27855" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D27855</a><br>
><br>
> Modified:<br>
> llvm/trunk/include/llvm/<wbr>Analysis/ValueTracking.h<br>
> llvm/trunk/lib/Analysis/<wbr>ValueTracking.cpp<br>
><br>
> Modified: llvm/trunk/include/llvm/<wbr>Analysis/ValueTracking.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/ValueTracking.h?rev=290786&r1=290785&r2=290786&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/include/<wbr>llvm/Analysis/ValueTracking.h?<wbr>rev=290786&r1=290785&r2=<wbr>290786&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/include/llvm/<wbr>Analysis/ValueTracking.h (original)<br>
> +++ llvm/trunk/include/llvm/<wbr>Analysis/ValueTracking.h Sat Dec 31 11:37:01 2016<br>
> @@ -308,12 +308,12 @@ template <typename T> class ArrayRef;<br>
> bool isKnownNonNull(const Value *V);<br>
><br>
> /// Return true if this pointer couldn't possibly be null. If the context<br>
> - /// instruction is specified, perform context-sensitive analysis and return<br>
> - /// true if the pointer couldn't possibly be null at the specified<br>
> - /// instruction.<br>
> + /// instruction and dominator tree are specified, perform context-sensitive<br>
> + /// analysis and return true if the pointer couldn't possibly be null at the<br>
> + /// specified instruction.<br>
> bool isKnownNonNullAt(const Value *V,<br>
> const Instruction *CtxI = nullptr,<br>
> - const DominatorTree *DT = nullptr);<br>
> + const DominatorTree *DT = nullptr);<br>
><br>
> /// Return true if it is valid to use the assumptions provided by an<br>
> /// assume intrinsic, I, at the point in the control-flow identified by the<br>
><br>
> Modified: llvm/trunk/lib/Analysis/<wbr>ValueTracking.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ValueTracking.cpp?rev=290786&r1=290785&r2=290786&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Analysis/ValueTracking.cpp?<wbr>rev=290786&r1=290785&r2=<wbr>290786&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/lib/Analysis/<wbr>ValueTracking.cpp (original)<br>
> +++ llvm/trunk/lib/Analysis/<wbr>ValueTracking.cpp Sat Dec 31 11:37:01 2016<br>
> @@ -3368,6 +3368,8 @@ static bool isKnownNonNullFromDominating<br>
> const DominatorTree *DT) {<br>
> assert(V->getType()-><wbr>isPointerTy() && "V must be pointer type");<br>
> assert(!isa<ConstantData>(V) && "Did not expect ConstantPointerNull");<br>
> + assert(CtxI && "Context instruction required for analysis");<br>
> + assert(DT && "Dominator tree required for analysis");<br>
><br>
> unsigned NumUsesExplored = 0;<br>
> for (auto *U : V->users()) {<br>
> @@ -3410,7 +3412,10 @@ bool llvm::isKnownNonNullAt(const Value<br>
> if (isKnownNonNull(V))<br>
> return true;<br>
><br>
> - return CtxI ? ::<wbr>isKnownNonNullFromDominatingCo<wbr>ndition(V, CtxI, DT) : false;<br>
> + if (!CtxI || !DT)<br>
> + return false;<br>
> +<br>
<br>
</div></div>How hard is to guarantee that we always have a valid/updated dominator<br>
when we get here (i.e. we don't need to bail out)?<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Davide<br>
<br>
"There are no solved problems; there are only problems that are more<br>
or less solved" -- Henri Poincare<br>
</font></span></blockquote></div><br></div>