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

[Tyker via Phabricator via llvm-commits]

Tyker added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:679
+    }
+
   for (auto &AssumeVH : Q.AC->assumptionsFor(V)) {
----------------
jdoerfert wrote:
> Tyker wrote:
> > 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.
> > 
> This is fine with me. We might want to add a helper that adds this "common" filter (`return isValidAssumeForContext(Assume, Q.CxtI, Q.DT);`) given an instruction (`CtxI`) and an optional dom tree.
isValidAssumeForContext is in Analysis so we can't access it from KnowledgeRetention.


================
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:
> Tyker wrote:
> > 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.
> > for AssumptionCache. it could be used here but i think it would be costlier to use it than what we currently have.
>  I am not sure how using assumptionCache would be more costly, assuming it is already populated?
> 
> > 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).
> 
> But you still have to check all uses of each value (of which many may not be assumptions), to find the available assumptions IIUC. By using AssumptionCache, you can skip the traversal of the use list, which should be quite beneficial, assuming most values do not have associated assumptions.
> 
when we lookup from use, we can use the operand number so we can use getKnowledgeFromOperandInAssume which is expected to be cheaper than log N for N being the number of element in the assume. if we don't have the operand number, we have to fallback to linear search.

that said i think we can be get the best of both world by improving AssumptionCache to store the operand bundle index with the assume. but this should probably go into an other patch.


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

https://reviews.llvm.org/D76149