[PATCH] D27585: [CaptureTracking] Add optimistic capture tracker for stores

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D27585#621980, @JDevlieghere wrote:

> In https://reviews.llvm.org/D27585#621863, @hfinkel wrote:
>
> > > Otherwise, it only consider %ptr to escape in the previous example if it is stored to either a global or to one of the arguments of the function.
> >
> > I suppose this is named "optimistic" because it might return a false negative? If the pointer value is stored into some alloca then read and the read value stored into some global, then the value is captured but your analysis will return false. Is this what you intend?
>
>
> Does the example below cover what you have in mind?
>
>   @global = external global i32*
>  
>   define void @sample() {
>   entry:
>     %ptr = alloca i32
>     store i32 1, i32* %ptr
>     %ptrtoptr = alloca i32*
>     store i32* %ptr, i32** %ptrtoptr
>     %deref = load i32*, i32** %ptrtoptr
>     store i32* %deref , i32** @global
>     ret void
>   }
>
>
> If so, the escape of `%ptr` is properly detected by my change. I have a small printer pass for capture tracking which enables me to easily verify this kind of stuff. I will update my diff to include it, we can always remove it again later.


Can you please explain how this is detected? The store is to an alloca, and I don't believe that the system does any kind of memory-based data-flow tracking.

In any cases, some regression test cases will be necessary.



================
Comment at: lib/Analysis/CaptureTracking.cpp:81
+
+        if (auto *V = dyn_cast<Value>(&Arg)) {
+          if (!AA->isNoAlias(Ptr, V)) {
----------------
An Argument is always a Value, so this cast/check should never be necessary.


================
Comment at: lib/Analysis/CaptureTracking.cpp:82
+        if (auto *V = dyn_cast<Value>(&Arg)) {
+          if (!AA->isNoAlias(Ptr, V)) {
+            return true;
----------------
This check won't work correctly if the value is stored into some offset based on the argument. You can make the right kind of query, but only if you construct a Location with an unknown size based on the value.


================
Comment at: lib/Analysis/CaptureTracking.cpp:90
+      for (auto &Global : F->getParent()->globals()) {
+        if (auto *V = dyn_cast<Value>(&Global)) {
+          if (!AA->isNoAlias(Ptr, V)) {
----------------
Again, this cast/check is not necessary (a Global is always a Value).


================
Comment at: lib/Analysis/CaptureTracking.cpp:91
+        if (auto *V = dyn_cast<Value>(&Global)) {
+          if (!AA->isNoAlias(Ptr, V)) {
+            return true;
----------------
Same comment here about the size/offset issue.


================
Comment at: lib/Analysis/CaptureTracking.cpp:101
+    bool captured(const Use *U) override {
+
+      if (isa<ReturnInst>(U->getUser()) && !ReturnCaptures)
----------------
Remove this blank line.


================
Comment at: lib/Analysis/CaptureTracking.cpp:223
+  auto& AA = AM.getResult<AAManager>(F);
+  for(auto& BB : F) {
+    for(auto& I : BB) {
----------------
Add a space in between the for and the ( [same for the lines below for for/if].


Repository:
  rL LLVM

https://reviews.llvm.org/D27585