[PATCH] D110245: [ConstantFolding] Fold ptrtoint(gep i8 null, x) -> x

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1376
+          constexpr bool CanFoldNonzeroInboundsNullGEPToZero = false;
+          if (CanFoldNonzeroInboundsNullGEPToZero && GEP->isInBounds()) {
+            // Any non-zero inbounds GEP on NULL is poison, so we can return 0.
----------------
nikic wrote:
> This doesn't belong here. If we'd like to make this fold, it should work directly on `gep null, x -> null`, without requiring a ptrtoint around it. (Also note that this fold is not correct if null is defined, e.g. in non-zero address space).
Yes I agree. I added that fold locally later but it caused lots of undesirable changes. 

Langref has the following sentence:
`The only in bounds address for a null pointer in the default address-space is the null pointer itself.`, and
```
null_pointer_is_valid
If null_pointer_is_valid is set, then the null address in address-space 0 is considered to be a valid address for memory loads and stores. Any analysis or optimization should not treat dereferencing a pointer to null as undefined behavior in this function. Note: Comparing address of a global variable to null may still evaluate to false because of a limitation in querying this attribute inside constant expressions.
```
So I guess that implies `gep inbounds null` is valid if that attribute is set? Should the GEP section be changed to something like
`The only in bounds address for a null pointer in the default address-space is the null pointer itself. This does not apply if the null_pointer_is_valid attribute was used.`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110245