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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I have minor comments but I think this is fine. Please wait on the approval of @fhahn .



================
Comment at: llvm/include/llvm/IR/KnowledgeRetention.h:136
+getRetainedKnowledgeFromUse(const Use *U,
+                            ArrayRef<Attribute::AttrKind> AttrKinds);
+
----------------
`Return true iff U is a use ... with an attribute in AttrKinds.` There is no predicate.


================
Comment at: llvm/include/llvm/IR/KnowledgeRetention.h:144
+    llvm::function_ref<bool(RetainedKnowledge, Instruction *)> Filter =
+        [](RetainedKnowledge, Instruction *) { return true; });
+
----------------
-` and this assume dominate the instruction Ctx.`

The above description matches a convenient wrapper we should add (I also mention that below).


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:679
+    }
+
   for (auto &AssumeVH : Q.AC->assumptionsFor(V)) {
----------------
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.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:675
+      return true;
+  }
+
----------------
I don't think `DT` is mandatory here (line 664).


================
Comment at: llvm/test/Analysis/ValueTracking/assume.ll:92
+  ret i32 %6
+}
----------------
Add test4b, as test4 but with:
`define i32 @test4b(i32* readonly %0, i1 %cond) "null-pointer-is-valid"="true" {`




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76149