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

Fri Apr 3 10:15:13 PDT 2020

fhahn added inline comments.

================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:679
+    }
+
   for (auto &AssumeVH : Q.AC->assumptionsFor(V)) {
----------------
Tyker wrote:
> fhahn wrote:
> > Tyker wrote:
> > > fhahn wrote:
> > > > 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.
> > > > 
> > > > That's unfortunate in terms of layering :(
> > > i agree
> > > 
> > > > I assume the reason this is in IR is because of the AssumeBuilder part, right?
> > > Yes this is why.
> > > 
> > > > 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)?
> > > 
> > > yeah i think this is going to happen. but there are too many constraints to satisfy them all, although we can satisfy more constraint than we currently do.
> > > a list of constraint is:
> > >  - Test for the Query part depend on the AssumeBuilder part.
> > >  - We would like the Query part to have access to Analysis for the situation we have here and many more i think.
> > >  - In rG57c964aaa76bfaa908398fbd9d8c9d6d19856859#909236, @nikic wrote:
> > >     "Are you sure IR is the right place for this? I don't think that we have any passes in there, apart from the IR verifier. Possibly this needs
> > >     splitting up into parts that are needed by analysis and by transforms?"
> > >  - When working on improvement on the assume builder i saw some situation where having access to Analysis is beneficial.
> > >  - The AssumeBuilder part doesn't logically fit in Analysis
> > > 
> > > I think that moving the Query part to Analysis is overall a good idea.
> > > however placing the AssumeBuilder part always break some constraint either it is in IR, Analysis or Transform.
> > > if placed in IR we have a pass in IR (for testing) which is subotimal and AssumeBuilder part can't use Analysis.
> > > if placed in Analysis it intuitively doesn't fit there.
> > > if placed in Transform we either need to change the testing method for the Query part or test it from Transform.
> > > Test for the Query part depend on the AssumeBuilder part.
> > 
> > Is that the main reason for being in IR/ currently? 
> > 
> > I think a better compromise would be to have the AsumeBuilder in Transforms/Utils or something and then test the query part in Transform/ there as well. Also, shouldn't it be also possible to test the query part without the AssumeBuilder, by specifying the expected assume bundles in the IR directly?
> > 
> > > In rG57c964aaa76bfaa908398fbd9d8c9d6d19856859#909236, @nikic wrote: "Are you sure IR is the right place for this? I don't think that we have any passes in there, apart from the IR verifier. Possibly this needs splitting up into parts that are needed by analysis and by transforms?"
> > 
> > +1 to moving the pass out of IR/.
> > 
> > > I think that moving the Query part to Analysis is overall a good idea.
> > 
> > Sounds good. That should be a relatively straight forward change I think.
> > 
> > > however placing the AssumeBuilder part always break some constraint either it is in IR, Analysis or Transform.
> > 
> > As mentioned above, if the main reason for it being in IR/ is the unit tests, I would recommend directly construction instruction with assume bundles for the tests in Analysis/ (if possible. I might miss some details here). Conceptually the pass should be somewhere in Transforms/. I think a comparable case is PredicateInfo, which lives in llvm/include/llvm/Transforms/Utils/PredicateInfo.h.
> > 
> this is going to be fixed by D77171.
> and this patch have been rebased on it.
> so i added the getKnowledgeValidInContext as requested.
SGTM, thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76149