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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I somehow have a bad feeling about this pattern matching "hack".

After looking at some of the test cases only, couldn't we rewrite `null` comparisons against a `gep inbounds` altogether in a more generic way?

I mean something like this, assuming `null` is not a valid pointer:

  %c = icmp eq, null, gep inbounds %p %idx
  %d = icmp ne, null, gep inbounds %p %idx

will become

  %c0 = icmp eq, ptrtoint(%p), 0
  %c1 = icmp eq,         %idx, 0
  %c = and %c0, %c1
  %d = xor %c, 1

Now, `%c0` is loop invariant and `%c1` is false for all but one iteration (in the common case of a counting loops).
In addition, if we can show `%p != null` we know `%c0` is false which makes `%c` false as well and `%d` true.

Maybe I made a mistake but if the above logic holds, I would prefer a solution like that in one or two steps, that is perform the transformation either way and then check if `%p` is `nonnull` or only do it if we know `%p` is `nonnull` (maybe in inst-combine)?



================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:219
+      // With inbounds, p + PosNum != nullptr;
+      // whereas p + NegNum == nullptr would imply p is out of bounds
+      if (SE->isKnownNonZero(Idx)) {
----------------
I am confused by this comment, especially the negative part.

Without thinking about it too much I would have said somehting like this:

```
// With inbounds, 'g = p + NonZero' implies both g and p are valid pointers. Assuming null is not a valid pointer in this function, see NullPointerIsDefined above, we know g and p are both non-null. Note, this does not hold if the offset is zero.
```


================
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)
----------------
nikic wrote:
> 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`.
assuming you meant the `inbounds` case and this is legal, which I can imagine it is, we might have missed this special case in other places as well.


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

https://reviews.llvm.org/D64533