[PATCH] D99642: For non-null pointer checks, do not descend through out-of-bounds GEPs

[Nuno Lopes via Phabricator via llvm-commits]

nlopes added a comment.

In D99642#2660864 <https://reviews.llvm.org/D99642#2660864>, @lebedev.ri wrote:

> In D99642#2660849 <https://reviews.llvm.org/D99642#2660849>, @fhahn wrote:
>
>> In D99642#2660824 <https://reviews.llvm.org/D99642#2660824>, @lebedev.ri wrote:
>>
>>> alive2 again doesn't agree that non-inbounds GEP is allowed to produce null pointer: https://alive2.llvm.org/ce/z/9wfL5x
>>
>> Interesting, but also surprising, especially because the LangRef explicitly calls out GEPs without `inbounds` to wrap silently?
>
> Yes.
> I think it's another example of alive2 trying to invent a memory model that isn't fit for the real world.

Let me try to explain our reasoning:

- GEP doesn't change the object of a pointer, just the offset within that object. Having this rule enables a lot of optimizations (e.g. you know "for free" that `gep %p, %x` and `gep %q, %y` can't alias if %p/%q are the result of e.g. distinct alloca/malloc, even if you know nothing about %x/%y).
- Pointer comparisons need to account for the previous rule plus how we lower pointer comparisons (an integer comparison in assembly). For pointers whose offsets are inbounds, integer comparison == object/offset comparison. If one of the pointers is OOB, then things get complicated because in assembly an OOB pointer may be equal to an inbounds pointer, while in IR we would rather not have that situation to enable optimizations. This lead us to pursue a semantics where the result of a comparison with an OOB ptr is non-deterministic, such that the IR can assume it's always false and assembly can sometimes produce true. The implementation in Alive2 is not complete FYI.

This model has some disadvantages, namely inhibiting some optimizations that LLVM does around null (and only that AFAICT). @aqjune is actively working on this, and testing different models to try to find out a good solution, but I can't say when it'll be ready. Hopefully within a couple of weeks. We are still collecting pointer comparison optimizations done by LLVM ATM (instcombine, SCEV, AA, etc).
This is our list of things that need to be taken into account: https://docs.google.com/document/d/1n2Hqw0bkWK_mUi1xEcK4ruLKE-xxTdQ4426KnWY1nHI/edit?usp=sharing



================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4275
     if (auto *GEP = dyn_cast<GEPOperator>(V)) {
+      if (InBounds && !GEP->isInBounds())
+        return V;
----------------
fhahn wrote:
> According to https://llvm.org/docs/LangRef.html#pointer-aliasing-rules, `inbounds` should not impact the underlying object property IIUC. But probably can only rely on that, if the pointer gets actually dereferenced? 
Agreed. GEP doesn't change the object even without inbounds.


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

https://reviews.llvm.org/D99642