[PATCH] D144563: [SimplifyCFG] Improve the precision of `PtrValueMayBeModified`

[DianQK via Phabricator via llvm-commits]

DianQK added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:7132
+            PtrValueMayBeModified = true;
+          }
+          if (!(GEP->isInBounds() &&
----------------
nikic wrote:
> The first block probably isn't supposed to be there?
Sorry I didn't get.
First, we only focus two cases:
a. getelementptr (TY, null, not zero)          -> maybe be modified
a. getelementptr inbounds (TY, null, not zero) -> poison?

The first
```
if (!GEP->isInBounds()) {
   PtrValueMayBeModified = true;
}
```
is the "a" case.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:7128
+        // getelementptr inbounds (TY, null, 0)        -> null
+        // getelementptr inbounds (TY, null, not zero) -> poison
+        if (!GEP->isInBounds() && !GEP->hasAllZeroIndices())
----------------
nikic wrote:
> nikic wrote:
> > This is only the case if `!NullPointerIsDefined(GEP->getFunction(), GEP->getPointerAddressSpace())`.
> Though we should probably just move that check to the start of the function, because we end up repeating it everywhere anyway.
I think it would be fine to call `NullPointerIsDefined` directly on each `gep`, `load` and `store`, because we still have to query address space.

Would it be cleaner to just use `NullPointerIsDefined`?


================
Comment at: llvm/test/Transforms/SimplifyCFG/UnreachableEliminate.ll:476
 ;
 entry:
   br i1 %X, label %if, label %else
----------------
jdoerfert wrote:
> I'm worried because I didn't see a test with inbounds gep and `@fn_noundef_arg` call. 
> `!GEP->hasAllZeroIndices()` does not imply non zero. It means we don't know it's all zero.
> Can we make sure we don't accidentally read `poison` into the situation where it could as well be `null`.
I have added the fn_noundef_arg case.
For all ptr cases, `PtrValueMayBeModified` is used to check if it always be null.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144563