[PATCH] D118673: [Attributor] Introduce the `AA::isPotentiallyReachable` helper APIs

[Kuter Dinel via Phabricator via llvm-commits]

kuter accepted this revision.
kuter added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:471
+        return true;
+      continue;
+    }
----------------
jdoerfert wrote:
> kuter wrote:
> > I don't think this would work for recursive functions  when doing backwards reachability.
> > the next instruction might contain a call that will reach `ToI`
> I don't understand what the problem is.
> If `CurFromI->getNextNode()` can reach `ToI`, the query `isAssumedReachable(A, *CurFromI, *ToI);` will be true.
> Other calls that end up in the `ToFn`, e.g., going through recursion, are handled by the continue here instead of a return false.
I think you are right. I need to sleep a little more : )


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:482
+                        << *CurFromI << " is not checked backwards, abort\n");
+      return true;
+    }
----------------
jdoerfert wrote:
> kuter wrote:
> > We haven't called `FnReachabilityAA.instructionCanReach` yet. This function won't do interprocedural reachability if there is no GoBackwardsCB  ?
> > 
> Yes, as the comment states, if there is no backwards CB we give up. Nothing right now will otherwise cause this to not return "may reach" anyway. We eventually hit an external function and the callers are unknown and we give up. Once we have logic to determine an internal function can only be reached from specific places, etc. we can do more, unclear if that will be here though.
I interpreted the comment as not doing any backwards search if there is no callback not "not doing any interprocedural reachability" but ok. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118673