[PATCH] D152241: [CaptureTracking] Do not capture equality compares of same object

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

I just realized that my comment on getUnderlyingObject() wasn't right in the following sense: While generally we can't assume that a comparison with the same underlying object is non-capturing, in this particular context, if the value were used in something like ptrmask, we would already report a capture at that point, so it would actually be fine to just use getUnderlyingObject() here.

The purpose of the getUnderlyingObject() check is effectively only to make sure that we don't handle cases like `(c ? alloca : other) == ...`, where the `other` contribution does not go through CaptureTracking. Everything else is already handled by the other CaptureTracking checks.

So I think your previous version was fine in that respect, though if you also want to handle `inbounds` GEP chains you'll need the separate helper anyway.

In D152241#4411708 <https://reviews.llvm.org/D152241#4411708>, @caojoshua wrote:

> Does not have to be part of this patch, but could we extend this to cover non-equality comparisons if all the GEPs are inbounds? From https://llvm.org/docs/LangRef.html#id234, inbounds GEPs are poison if there is wrapping

It's safe to handle inbounds GEP if the predicate is unsigned. Signed predicates can not be handled.

In D152241#4411731 <https://reviews.llvm.org/D152241#4411731>, @goldstein.w.n wrote:

> That being said, something like:
>
>   define i1 @src_gep(ptr %p, i64 %off) {
>     %p_gep = getelementptr inbounds i64, ptr %p, i64 %off
>     %cmp = icmp ugt ptr %p_gep, %p
>     ret i1 %cmp
>   }
>
>
> Already folds out the GEP entirely (and as a result makes `%p` unused) so not sure if you actually need to change anything to get the desired behavior.

I think the main interest here is in cases involving loop phis, in which case we don't reliably do this, I believe. (Though there is the "indexed compare" fold that does try.)



================
Comment at: llvm/lib/Analysis/CaptureTracking.cpp:82
+  if (!V->getType()->isPointerTy())
+    return V;
+  SmallDenseMap<const Value *, const Value *> Visited;
----------------
Unnecessary?


================
Comment at: llvm/lib/Analysis/CaptureTracking.cpp:85
+
+  std::function<const Value *(const Value *)> Visit = [&](const Value *V) {
+    const Value *NewV = V;
----------------
Don't use std::function


================
Comment at: llvm/lib/Analysis/CaptureTracking.cpp:91
+      NewV = Visit(GEP->getPointerOperand());
+    if (auto *PN = dyn_cast<PHINode>(V)) {
+      // We can look through PHIs if each underlying value has the same
----------------
If we handle phis, we should also handle selects.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152241