[PATCH] D154054: [CaptureTracking] Don't consider comparison of inbounds GEP with nonnull non-capturing

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
Herald added subscribers: StephenFan, hiraditya.
Herald added a project: All.
nikic requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

This is required to bring CaptureTracking in line with the new semantics from D154051 <https://reviews.llvm.org/D154051>, as `gep inbounds p, 0` is now always non-poison.

There are many ways in which the inbounds special case could be preserved: If the index is known non-zero, or there is an inbounds chain down to an identified object, etc. However, I have opted to drop the special case entirely, as it appears to be low value: In cases where we can determine such things (e.g. the affected test cases) we would end up removing the compare via isGEPKnownNonNull() logic anyway.

Depends on: D154051 <https://reviews.llvm.org/D154051>


https://reviews.llvm.org/D154054

Files:
  llvm/lib/Analysis/CaptureTracking.cpp
  llvm/test/Transforms/FunctionAttrs/nocapture.ll


Index: llvm/test/Transforms/FunctionAttrs/nocapture.ll
===================================================================
--- llvm/test/Transforms/FunctionAttrs/nocapture.ll
+++ llvm/test/Transforms/FunctionAttrs/nocapture.ll
@@ -488,7 +488,7 @@
 
 define i1 @nocaptureInboundsGEPICmp(ptr %x) {
 ; CHECK-LABEL: define i1 @nocaptureInboundsGEPICmp
-; CHECK-SAME: (ptr nocapture readnone [[X:%.*]]) #[[ATTR0]] {
+; CHECK-SAME: (ptr readnone [[X:%.*]]) #[[ATTR0]] {
 ; CHECK-NEXT:    [[TMP1:%.*]] = getelementptr inbounds i32, ptr [[X]], i32 5
 ; CHECK-NEXT:    [[TMP2:%.*]] = icmp eq ptr [[TMP1]], null
 ; CHECK-NEXT:    ret i1 [[TMP2]]
@@ -500,7 +500,7 @@
 
 define i1 @nocaptureInboundsGEPICmpRev(ptr %x) {
 ; CHECK-LABEL: define i1 @nocaptureInboundsGEPICmpRev
-; CHECK-SAME: (ptr nocapture readnone [[X:%.*]]) #[[ATTR0]] {
+; CHECK-SAME: (ptr readnone [[X:%.*]]) #[[ATTR0]] {
 ; CHECK-NEXT:    [[TMP1:%.*]] = getelementptr inbounds i32, ptr [[X]], i32 5
 ; CHECK-NEXT:    [[TMP2:%.*]] = icmp eq ptr null, [[TMP1]]
 ; CHECK-NEXT:    ret i1 [[TMP2]]
Index: llvm/lib/Analysis/CaptureTracking.cpp
===================================================================
--- llvm/lib/Analysis/CaptureTracking.cpp
+++ llvm/lib/Analysis/CaptureTracking.cpp
@@ -58,17 +58,16 @@
 bool CaptureTracker::shouldExplore(const Use *U) { return true; }
 
 bool CaptureTracker::isDereferenceableOrNull(Value *O, const DataLayout &DL) {
-  // An inbounds GEP can either be a valid pointer (pointing into
-  // or to the end of an allocation), or be null in the default
-  // address space. So for an inbounds GEP there is no way to let
-  // the pointer escape using clever GEP hacking because doing so
-  // would make the pointer point outside of the allocated object
-  // and thus make the GEP result a poison value. Similarly, other
-  // dereferenceable pointers cannot be manipulated without producing
-  // poison.
-  if (auto *GEP = dyn_cast<GetElementPtrInst>(O))
-    if (GEP->isInBounds())
-      return true;
+  // We want comparisons to null pointers to not be considered capturing,
+  // but need to guard against cases like gep(p, -ptrtoint(p2)) == null,
+  // which are equivalent to p == p2 and would capture the pointer.
+  //
+  // A dereferenceable pointer is a case where this is known to be safe,
+  // because the pointer resulting from such a construction would not be
+  // dereferenceable.
+  //
+  // It is not sufficient to check for inbounds GEP here, because GEP with
+  // zero offset is always inbounds.
   bool CanBeNull, CanBeFreed;
   return O->getPointerDereferenceableBytes(DL, CanBeNull, CanBeFreed);
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D154054.535682.patch
Type: text/x-patch
Size: 2623 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230629/29e188fa/attachment.bin>