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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:679
+    }
+
   for (auto &AssumeVH : Q.AC->assumptionsFor(V)) {
----------------
Tyker wrote:
> 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.
That's unfortunate in terms of layering :( 

I assume the reason this is in IR is because of the AssumeBuilder part, right?

Did you consider splitting the code into an analysis part (that lives in lib/Analysis and can share utilities with ValueTracking & co) & a transform part (that lives in either IR/Transforms)?

I think it would be great to share utilities with the existing code that deals with assumes, as that might lead to general improvements that also benefit the existing code, increases test coverage and reduces duplication.



================
Comment at: llvm/lib/IR/KnowledgeRetention.cpp:314
+      if (auto *Intr = dyn_cast<IntrinsicInst>(U.getUser()))
+        if (Intr->getIntrinsicID() == Intrinsic::assume) {
+          RetainedKnowledge RK =
----------------
Tyker wrote:
> 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.
Right, I see what you mean now. I guess it all depends on what we expect to be larger, the number of (non-assume) uses of the value or the number of operands in the assume. I would expect the former to be larger in practice, given that most values do not have any assume users.


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

https://reviews.llvm.org/D76149