[PATCH] D64533: [IndVars] Special case the problematic (gep inbounds p, iv == nullptr) problem (pr42357)

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:206
+        L->isLoopInvariant(GEP->getPointerOperand()) &&
+        GEP->getType()->isPointerTy()) {
+      IRBuilder<> B(ICmp);
----------------
reames wrote:
> nikic wrote:
> > `GEP->getType()->isPointerTy()` should always be true for a single-index GEP ... right?
> Nope.  The following is valid:
> %vec_gep = gep i8, i8* %scalar_base, <2 x i64> zeroinitializer
> 
> (That's equivalent to a broadcast.)
Oh right, thanks.


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:220
+      // whereas p + NegNum == nullptr would imply p is out of bounds
+      if (SE->isKnownNonZero(Idx)) {
+        if (Pred == ICmpInst::ICMP_NE)
----------------
reames wrote:
> nikic wrote:
> > I believe we also need to check for a pointer to a zero-sized type here, in which case we might have `null + 0*Idx == null` even if the index is non-zero.
> Just to make sure I'm following you, you concern is a zero-sized *index type* right?  If so, no, that's disallowed by the verifier.  
> 
> 
I'm not sure what the right terminology is, but I mean something like `getelementptr {}, {}* %p, i32 %idx`, which is legal and evaluates to `{}* %p`.


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

https://reviews.llvm.org/D64533