[PATCH] D76149: [AssumeBundles] Use assume bundles in isKnownNonZero

[Tyker via Phabricator via llvm-commits]

Tyker marked 3 inline comments as done.
Tyker added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:679
+    }
+
   for (auto &AssumeVH : Q.AC->assumptionsFor(V)) {
----------------
jdoerfert wrote:
> jdoerfert wrote:
> > I feel we need to move some/all of this complexity into `KnowledgeRetention`.
> > Ideally, one (or at least I) would like something along the lines of:
> > 
> > ```
> > if (Q.DT && Q.CtxI) {
> > SmallVector<AttributeKind, 2> AttrKinds;
> > AttrKinds.append(Attribute::NonNull);
> > if (llvm::NullPointerIsDefined(Q.CtxI->getFunction(), V->getType()->getPointerAddressSpace()))
> >    AttrKinds.append(Attribute::Dereferenceable);
> > if (hasKnownAttributeDominatingInst(*V, AttrKinds, *Q.DT, *Q.CtxI))
> >   return true;
> > }
> > ```
> > 
> > The impl of `hasKnownAttributeDominatingInst` does the required steps, thus looking at the uses. Actually all the steps should be in `getKnownNonNullAndDerefBytesForUse` which lives in Attributor.cpp. It does actually more, e.g., use in a call site or and recursive queries via the Attributor, but we should somehow share this logic.
> > 
> > My suggestion:
> >  1) Something similar to the above interface in KnowledgeRetention
> >  2) Make Attributor etc. in  `getKnownNonNullAndDerefBytesForUse` optional.
> >  3) Teach `getKnownNonNullAndDerefBytesForUse` about assume uses.
> >  3) Make the `getKnownNonNullAndDerefBytesForUse` logic public and invoke it if we have an attribute kind NonNull or Dereferenceable[_or_null]. 
> > 
> > WDYT?
> Maybe `hasKnownAttributeDominatingInst` should not need to look for `deref` at all as we derive it from `deref` in the Attributor. Otherwise we could make a helper to encapsulate the `NullPointerIsDefined` stuff.
i adapted the  getKnownNonNullAndDerefBytesForUse to use the same code. but i am not sure this matches what you had in mind.

> Maybe hasKnownAttributeDominatingInst should not need to look for deref at all as we derive it from deref in the Attributor. Otherwise we 
> could make a helper to encapsulate the NullPointerIsDefined stuff.

in ValueTracking this is not needed if the attributor has run before. but i am not sure all places that can add a deref attribute will add a nonnull as well when they can. if you are confident i am fine with not looking for deref.
in the attributor we should definitely look at both nonnull and deref.



================
Comment at: llvm/lib/IR/KnowledgeRetention.cpp:314
+      if (auto *Intr = dyn_cast<IntrinsicInst>(U.getUser()))
+        if (Intr->getIntrinsicID() == Intrinsic::assume) {
+          RetainedKnowledge RK =
----------------
fhahn wrote:
> There's AssumptionCache, which is used in various places that inspect assumes, including ValueTracking. Is there a reason to not use it here?
> 
>  If there is a layering problem, I think that would need a bit more thought, but ideally we would re-use the existing infrastructure to make things available as widely as possible.
isValidAssumeForContext is now used and i saw the improvement in a test case.

for AssumptionCache. it could be used here but i think it would be costlier to use it than what we currently have.
we are working with knowledge in operand bundles, the assume is always the direct user of the value it knows something about (which is not true for boolean assumes). so we can iterate over all uses (non-recursively) to find all candidate llvm.assume. looking up the knowledge in the assume is cheaper when we have the the operand number which is only available when we lookup from the uses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76149