[PATCH] D124677: [ConstantFold] Don't convert getelementptr to ptrtoint+inttoptr

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added a comment.

In D124677#3483937 <https://reviews.llvm.org/D124677#3483937>, @nikic wrote:

> In D124677#3483890 <https://reviews.llvm.org/D124677#3483890>, @aqjune wrote:
>
>> 
>
> I don't think this follows: This is talking about "integer constant expressions", which are a front-end concern. It means that the front-end is required to match for `(void*)0` and emit that as `ptr null` rather than `inttoptr (i64 0 to ptr)`. At least from that wording, doing `int x = 0; (void*)x`does not result in a null pointer (though possibly other wording implies that?)
>
> I don't think having `ptrtoint 1` have universal provenance and `ptrtoint 0` have nullary provenance can lead to consistent semantics. It renders many transforms that are "obviously correct" illegal, such as:
>
>   define ptr @src(ptr %p, i64 %idx) {
>     %p2 = getelementptr i8, ptr %p, i64 %idx
>     ret ptr %p2
>   }
>   define ptr @tgt(ptr %p, i64 %idx) {
>     %p.int = ptrtoint ptr %p to i64
>     %p.add = add i64 %p.int, %idx
>     %p2 = inttoptr i64 %p.add to ptr
>     ret ptr %p2
>   }
>
> While this transform is very undesirable, it should be correct because it only increases provenance. However, due to the special `ptrtoint 0` handling this is incorrect for the special case where `p.int == -idx`.

One alternative solution is to define a null pointer as having universal provenance.
This is against the LangRef wording `A null pointer in the default address-space is associated with no address.` (meaning that null + x must not be able to access anything), but explains these:

- Replacing a pointer into NULL is supported because it makes the program more defined.

  if (x == null) {
    f(x); -> f(null);
  }

- Folding `inttoptr 0` to `null` is naturally explained.

Instead, we will lose optimization opportunities on `gep null, x`. However, alias analysis can still support `gep inbounds null, x` because the users of this pointer must ensure that range [null, null+x] are inbounds, and no allocation is located at null.
Therefore, it comes to preserving the flag `inbounds` as much as possible.

IMO the semantics of NULL pointer is also in the gray area; didn't mean to be against this patch, but wanted to claim that inttoptr 0 -> NULL might be another issue.


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

https://reviews.llvm.org/D124677