<div dir="ltr"><div>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?<br><br>For reference, my proposed value tracking change is here:<br><a href="https://reviews.llvm.org/D28204">https://reviews.llvm.org/D28204</a></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 3, 2017 at 4:36 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"><span class="">On Sat, Dec 31, 2016 at 10:30 AM, Sanjay Patel <<a href="mailto:spatel@rotateright.com">spatel@rotateright.com</a>> wrote:<br>
> I think it's not bad; there are only 5 calls to isKnownNonNullAt() in the<br>
> whole project. So we could just hoist those checks to the callers as needed<br>
> and assert in here instead. Let me know if I've understood the suggestion.<br>
><br>
<br>
</span>I mean something like this:<br>
<br>
```<br>
bool runOnFunction(Function &F, DominatorTree *DT, ...) {<br>
assert(DT && "Dominator Tree required");<br>
[...]<br>
}<br>
```<br>
(i.e. we always guarantee that the domtree/AA/* we have is valid in<br>
order to guarantee that optimization will fire and we never bail out).<br>
Not sure how hard would be. Thoughts?<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
> What I was hoping to do next is change the call to isKnownNonNull() that is<br>
> currently in isKnownNonZero():<br>
> <a href="https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/ValueTracking.cpp#L1834" rel="noreferrer" target="_blank">https://github.com/llvm-<wbr>mirror/llvm/blob/master/lib/<wbr>Analysis/ValueTracking.cpp#<wbr>L1834</a><br>
><br>
> into a call to isKnownNonNullAt(), so we'd have to add guard checks in the<br>
> 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>
> <a href="https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/ValueTracking.cpp#L3409" rel="noreferrer" target="_blank">https://github.com/llvm-<wbr>mirror/llvm/blob/master/lib/<wbr>Analysis/ValueTracking.cpp#<wbr>L3409</a><br>
> ...for non-context-sensitive analysis only in isKnownNonNullAt(), but not in<br>
> isKnownNonNull(), so I may not be understanding how this all works.<br>
><br>
> On Sat, Dec 31, 2016 at 10:58 AM, Davide Italiano <<a href="mailto:davide@freebsd.org">davide@freebsd.org</a>><br>
> wrote:<br>
>><br>
>> 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<br>
>> > isKnownNonNullFromDominatingCo<wbr>ndition(); NFCI<br>
>> ><br>
>> > I don't think this hole is currently exposed, but I crashed regression<br>
>> > tests for<br>
>> > jump-threading and loop-vectorize after I added calls to<br>
>> > 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,<br>
>> > 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:<br>
>> > <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>
>> ><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<br>
>> > 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<br>
>> > context<br>
>> > - /// instruction is specified, perform context-sensitive analysis and<br>
>> > return<br>
>> > - /// true if the pointer couldn't possibly be null at the specified<br>
>> > - /// instruction.<br>
>> > + /// instruction and dominator tree are specified, perform<br>
>> > context-sensitive<br>
>> > + /// analysis and return true if the pointer couldn't possibly be null<br>
>> > 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<br>
>> > by the<br>
>> ><br>
>> > Modified: llvm/trunk/lib/Analysis/<wbr>ValueTracking.cpp<br>
>> > URL:<br>
>> > <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>
>> ><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<br>
>> > *DT) {<br>
>> > assert(V->getType()-><wbr>isPointerTy() && "V must be pointer type");<br>
>> > assert(!isa<ConstantData>(V) && "Did not expect<br>
>> > 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) :<br>
>> > false;<br>
>> > + if (!CtxI || !DT)<br>
>> > + return false;<br>
>> > +<br>
>><br>
>> 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>
>><br>
>> --<br>
>> Davide<br>
>><br>
>> "There are no solved problems; there are only problems that are more<br>
>> or less solved" -- Henri Poincare<br>
><br>
><br>
<br>
<br>
<br>
--<br>
Davide<br>
<br>
"There are no solved problems; there are only problems that are more<br>
or less solved" -- Henri Poincare<br>
</div></div></blockquote></div><br></div>