[PATCH] D54956: [ValueTracking] Look through casts when determining non-nullness

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1714
   if (V->getType()->isPointerTy() && C->isNullValue() &&
-      isKnownNonZero(V->stripPointerCasts(), DL)) {
+      isKnownNonZero(V, DL)) {
     if (Pred == ICmpInst::ICMP_EQ)
----------------
fhahn wrote:
> jdoerfert wrote:
> > I'm not sure this is the right fix but I know we have to do something to pass "callsite_nonnull_args_through_casts.ll" with -O3 (even without my modifications below!).
> > The alternative, making sure Value::stripPointerCasts() is not stripping of AddrSpaceCasts, makes more and more sense to me as I always assumed it not to change "the value".
> I think this is an independent issue and should be addressed separately, assuming the test passes with instcombine.
Agreed, I'll revert the change. This should be part of the discussion about stripPointerCasts and its tendency to look through AddrSpaceCasts.


================
Comment at: lib/Analysis/ValueTracking.cpp:2024
+    // Look through various operations and instructions which do not alter the
+    // value, or at least not the nullness property of the value (sext/zext).
+    if (const BitCastOperator *BCO = dyn_cast<BitCastOperator>(V))
----------------
jdoerfert wrote:
> spatel wrote:
> > jdoerfert wrote:
> > > fhahn wrote:
> > > > The dyn_casts are not strictly necessary I think and using a switch on the opcode might be a little bit more compact, together with getOperand(0). But it doesn't really make a big difference.
> > > The switch is not much nicer, I think. Especially since I don't know how to add the operators, e.g., constant bitcast expressions, in the switch. Does anybody feel strongly about this?
> > Seems ok to me, but if we're going to dyn_cast, you can use 'auto *' to make it less wordy:
> > http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> > 
> > Another option to shrink the code would be something like this:
> >   Value *X;
> >   if (match(II, m_CombineOr(m_ZExtOrSExt(m_Value(X)), m_BitCast(m_Value(X)))))
> >     return isKnownNonZero(X, Depth, Q);
> > 
> > We're missing an m_IntToPtr() matcher...if we added that, all of the cases could use matchers.
> I can do `auto *` and I can try to implement it in a switch but I don't feel qualified to use the matcher logic just now. We also would need to compare "sizes" for int2ptr/ptr2int.
I use `auto *` in the next version.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54956