[PATCH] D71660: [ValueTracking] isKnownNonZero() should take non-null-ness assumptions into consideration (PR43267)

Wed Dec 18 08:23:19 PST 2019

lebedev.ri added a comment.

In D71660#1789768 <https://reviews.llvm.org/D71660#1789768>, @nikic wrote:

> In D71660#1789759 <https://reviews.llvm.org/D71660#1789759>, @lebedev.ri wrote:
>
> > In D71660#1789726 <https://reviews.llvm.org/D71660#1789726>, @nikic wrote:
> >
> > > Have you considered integrating this in `isKnownNonNullFromDominatingCondition()` instead? It already iterates over all users of `V != 0` style conditions and could easily consider assumes in addition to branches.
> > >  That may be more efficient than handling it separately.
> >
> >
> > I'm not sure why it would be better than relying on the infrastructure specifically designed to handling assumptions?
> >  It isn't obvious to me which approach is more performant, without benchmarks either approach may be much worse than the other one.
>
>
> It's a question of which existing infrastructure you want to reuse ;) `isKnownNonNullFromDominatingCondition()` already handles many other scenarios involving dominating conditions and assume-like operations, but misses a check for the actual assumes.
>
> In particular https://github.com/llvm-mirror/llvm/blob/2c4ca6832fa6b306ee6a7010bfb80a3f2596f824/lib/Analysis/ValueTracking.cpp#L1965-L1968 already deals with `llvm.experimental.guard()`. Adding support for assumes should be as simple as replacing `isGuard()` with `isGuard() || isAssume()` there.
>
> Does that make sense?

I get that. The question is, given we have `AssumptionCache` abstraction, why do we want to avoid it.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71660/new/

https://reviews.llvm.org/D71660